[BZ,17542] sunrpc: conditional jump depends on uninitialised value in svc_getreq_common
diff mbox

Message ID 54597868.3060408@redhat.com
State New
Headers show

Commit Message

Brad Hubbard Nov. 5, 2014, 1:07 a.m. UTC
If xports is NULL in xprt_register we malloc it but if sock >
_rpc_dtablesize() that memory does not get initialised and may in theory
contain any value. Later we make a conditional jump in svc_getreq_common
based on the uninitialised memory and this caused a general protection
fault in rpc.statd on an older version of glibc but this code has not
changed since that version.

Following is the valgrind warning.

==26802== Conditional jump or move depends on uninitialised value(s)
==26802==    at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so)
==26802==    by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so)
==26802==    by 0x10DE1F: ??? (in /sbin/rpc.statd)
==26802==    by 0x10D0EF: main (in /sbin/rpc.statd)
==26802==  Uninitialised value was created by a heap allocation
==26802==    at 0x4C2210C: malloc (vg_replace_malloc.c:195)
==26802==    by 0x53438BE: xprt_register (in /lib64/libc-2.5.so)
==26802==    by 0x53450DF: svcudp_bufcreate (in /lib64/libc-2.5.so)
==26802==    by 0x10FE32: ??? (in /sbin/rpc.statd)
==26802==    by 0x10D13E: main (in /sbin/rpc.statd)

I believe the solution here is to change the malloc call to a calloc
call and the attached patch does that. The GPF could not be reproduced
with the patched glibc.

2014-11-05  Brad Hubbard  <bhubbard@redhat.com>

	* sunrpc/svc.c: Resolve uninitialised xports in xprt_register

---
  sunrpc/svc.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andreas Schwab Nov. 5, 2014, 9:03 a.m. UTC | #1
Brad Hubbard <bhubbard@redhat.com> writes:

> Following is the valgrind warning.
>
> ==26802== Conditional jump or move depends on uninitialised value(s)
> ==26802==    at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so)

> ==26802==    by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so)

Why was svc_getreqset called with file descriptors that were never seen
by xprt_register?

Andreas.
Siddhesh Poyarekar Nov. 5, 2014, 9:14 a.m. UTC | #2
On Wed, Nov 05, 2014 at 10:03:46AM +0100, Andreas Schwab wrote:
> Brad Hubbard <bhubbard@redhat.com> writes:
> 
> > Following is the valgrind warning.
> >
> > ==26802== Conditional jump or move depends on uninitialised value(s)
> > ==26802==    at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so)
> 
> > ==26802==    by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so)
> 
> Why was svc_getreqset called with file descriptors that were never seen
> by xprt_register?

That is likely an application bug, but it might not be a bad idea to
include the patch anyway.  Failing the NULL check and returning seems
better than allowing to dereference arbitrary pointer values.

Siddhesh
Brad Hubbard Nov. 5, 2014, 9:24 a.m. UTC | #3
On 11/05/2014 07:03 PM, Andreas Schwab wrote:
> Brad Hubbard <bhubbard@redhat.com> writes:
>
>> Following is the valgrind warning.
>>
>> ==26802== Conditional jump or move depends on uninitialised value(s)
>> ==26802==    at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so)
>
>> ==26802==    by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so)
>
> Why was svc_getreqset called with file descriptors that were never seen
> by xprt_register?

Yes, I see your point of course but I still think the patch has merit.

My initial analysis was flawed.
Andreas Schwab Nov. 5, 2014, 9:31 a.m. UTC | #4
Siddhesh Poyarekar <siddhesh@redhat.com> writes:

> On Wed, Nov 05, 2014 at 10:03:46AM +0100, Andreas Schwab wrote:
>> Brad Hubbard <bhubbard@redhat.com> writes:
>> 
>> > Following is the valgrind warning.
>> >
>> > ==26802== Conditional jump or move depends on uninitialised value(s)
>> > ==26802==    at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so)
>> 
>> > ==26802==    by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so)
>> 
>> Why was svc_getreqset called with file descriptors that were never seen
>> by xprt_register?
>
> That is likely an application bug, but it might not be a bad idea to
> include the patch anyway.  Failing the NULL check and returning seems
> better than allowing to dereference arbitrary pointer values.

But what does this have to do with "sock > _rpc_dtablesize()"?

Andreas.
Siddhesh Poyarekar Nov. 5, 2014, 9:37 a.m. UTC | #5
On Wed, Nov 05, 2014 at 10:31:31AM +0100, Andreas Schwab wrote:
> > That is likely an application bug, but it might not be a bad idea to
> > include the patch anyway.  Failing the NULL check and returning seems
> > better than allowing to dereference arbitrary pointer values.
> 
> But what does this have to do with "sock > _rpc_dtablesize()"?

Nothing.  That analysis is incorrect AFAIK and it seems to be a
failure in calling xprt_register as you pointed out, which should be
an application bug.

Siddhesh
Ondřej Bílka Dec. 9, 2014, 10:33 p.m. UTC | #6
On Wed, Nov 05, 2014 at 02:44:34PM +0530, Siddhesh Poyarekar wrote:
> On Wed, Nov 05, 2014 at 10:03:46AM +0100, Andreas Schwab wrote:
> > Brad Hubbard <bhubbard@redhat.com> writes:
> > 
> > > Following is the valgrind warning.
> > >
> > > ==26802== Conditional jump or move depends on uninitialised value(s)
> > > ==26802==    at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so)
> > 
> > > ==26802==    by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so)
> > 
> > Why was svc_getreqset called with file descriptors that were never seen
> > by xprt_register?
> 
> That is likely an application bug, but it might not be a bad idea to
> include the patch anyway.  Failing the NULL check and returning seems
> better than allowing to dereference arbitrary pointer values.
> 
As its better to always fail than only sometimes I also think its good
to include it.
Brad Hubbard Dec. 9, 2014, 10:48 p.m. UTC | #7
On 12/10/2014 08:33 AM, Ondřej Bílka wrote:
> On Wed, Nov 05, 2014 at 02:44:34PM +0530, Siddhesh Poyarekar wrote:
>> On Wed, Nov 05, 2014 at 10:03:46AM +0100, Andreas Schwab wrote:
>>> Brad Hubbard <bhubbard@redhat.com> writes:
>>>
>>>> Following is the valgrind warning.
>>>>
>>>> ==26802== Conditional jump or move depends on uninitialised value(s)
>>>> ==26802==    at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so)
>>>
>>>> ==26802==    by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so)
>>>
>>> Why was svc_getreqset called with file descriptors that were never seen
>>> by xprt_register?
>>
>> That is likely an application bug, but it might not be a bad idea to
>> include the patch anyway.  Failing the NULL check and returning seems
>> better than allowing to dereference arbitrary pointer values.
>>
> As its better to always fail than only sometimes I also think its good
> to include it.
>

Does anyone want me to follow up on the upstream bug?

Any advice on how best to do so? I assume a "ping" in the Bugzilla and the
mailing list but what format should they have (definite noob here but I want to
learn).

Any advice is greatly appreciated.
Siddhesh Poyarekar Dec. 10, 2014, 7:38 a.m. UTC | #8
On Wed, Dec 10, 2014 at 08:48:20AM +1000, Brad Hubbard wrote:
> >As its better to always fail than only sometimes I also think its good
> >to include it.
> >

Andreas, do you object to me committing this patch?

Siddhesh
Andreas Schwab Dec. 10, 2014, 8:26 a.m. UTC | #9
Siddhesh Poyarekar <siddhesh@redhat.com> writes:

> On Wed, Dec 10, 2014 at 08:48:20AM +1000, Brad Hubbard wrote:
>> >As its better to always fail than only sometimes I also think its good
>> >to include it.
>> >
>
> Andreas, do you object to me committing this patch?

We should first understand what is the real bug.

Andreas.
Siddhesh Poyarekar Dec. 10, 2014, 8:36 a.m. UTC | #10
On Wed, Dec 10, 2014 at 09:26:26AM +0100, Andreas Schwab wrote:
> We should first understand what is the real bug.

The real bug is in the calling application, which calls svc_getreq on
a file descriptor that it did not register.  We're patching this in
sunrpc so that the check in svc_getreq_common below that was supposed
to catch such conditions actually works.

...
  xprt = xports[fd];
  /* Do we control fd? */
  if (xprt == NULL)
     return;
...

Siddhesh
Siddhesh Poyarekar March 2, 2015, 9:12 a.m. UTC | #11
On Wed, Dec 10, 2014 at 02:06:45PM +0530, Siddhesh Poyarekar wrote:
> On Wed, Dec 10, 2014 at 09:26:26AM +0100, Andreas Schwab wrote:
> > We should first understand what is the real bug.
> 
> The real bug is in the calling application, which calls svc_getreq on
> a file descriptor that it did not register.  We're patching this in
> sunrpc so that the check in svc_getreq_common below that was supposed
> to catch such conditions actually works.
> 
> ...
>   xprt = xports[fd];
>   /* Do we control fd? */
>   if (xprt == NULL)
>      return;
> ...

Andreas, is this OK or are you still not convinced about adding this
patch in?

Siddhesh
Andreas Schwab March 2, 2015, 9:29 a.m. UTC | #12
Siddhesh Poyarekar <siddhesh@redhat.com> writes:

> Andreas, is this OK or are you still not convinced about adding this
> patch in?

How about fixing the real bug?

Andreas.
Siddhesh Poyarekar March 2, 2015, 9:34 a.m. UTC | #13
On Mon, Mar 02, 2015 at 10:29:31AM +0100, Andreas Schwab wrote:
> Siddhesh Poyarekar <siddhesh@redhat.com> writes:
> 
> > Andreas, is this OK or are you still not convinced about adding this
> > patch in?
> 
> How about fixing the real bug?

Carlos has posted a fix for nfs-utils[1].  The intent of including
this patch is not to work around that specific bug, but to make the
code robust against such bugs in future.

Siddhesh

[1] http://article.gmane.org/gmane.linux.nfs/69437
Carlos O'Donell March 5, 2015, 8:05 p.m. UTC | #14
On 03/02/2015 04:34 AM, Siddhesh Poyarekar wrote:
> On Mon, Mar 02, 2015 at 10:29:31AM +0100, Andreas Schwab wrote:
>> Siddhesh Poyarekar <siddhesh@redhat.com> writes:
>>
>>> Andreas, is this OK or are you still not convinced about adding this
>>> patch in?
>>
>> How about fixing the real bug?
> 
> Carlos has posted a fix for nfs-utils[1].  The intent of including
> this patch is not to work around that specific bug, but to make the
> code robust against such bugs in future.
> 
> Siddhesh
> 
> [1] http://article.gmane.org/gmane.linux.nfs/69437
> 

This should be committed to glibc.

This is now committed to nfs-utils. I'd say we should make SunRPC
robust also, just as a matter of fact. We still have old applications
using the compatibility symbols that can benefit from having unregistered
fds be ignored.

commit 810423415dd1a2b7275b3abf294e6a69951614a1
Author: Carlos O'Donell <carlos@redhat.com>
Date:   Thu Feb 26 14:13:26 2015 -0500

    rpc.statd: Avoid passing unregistered socket to svc_getreqset
    
    rpc.statd may crash if it receives both a notification reply and a
    client connection at the same time. It crashes because it adds
    sockfd to SVC_FDSET and that violates the API contract.
    
    The SVC_FDSET is to be considered read-only and must not be modified
    by user code. The daemon modifies it for expediency to avoid
    having to maintain two distinct fd lists and select on each one.
    It is a practical choice that makes sense.
    
    Thus, if a notification reply arrives by itself everything works,
    or if a client connection arrives by itself everything works. Both
    must arrive at the same time for sockfd to be set in SVC_FDSET
    and to be processed by svc_getreqset because more than one of
    readfds is ready.
    
    It is the processing by svc_getreqset that will crash when it finds an
    unregistered fd in the list that doesn't correlate to any of the
    internal book keeping done by the library. At present the glibc
    SunRPC library will crash, but TIRPC does not (it is robust against
    invalid API usage in this case). However, future RPC libraries
    may be implemented differently, and the questionable API usage
    should be fixed.
    
    The simplest fix is for process_reply to *clear* sockfd from the
    ready-to-read fds, since it was never registered with xprt_register.
    This works because the code always calls process_reply before handing
    the fd set to the RPC layer for processing.
    
    Compile-tested on x86_64 against master.
    
    Signed-off-by: Carlos O'Donell <carlos@redhat.com>
    Signed-off-by: Steve Dickson <steved@redhat.com>

Cheers,
Carlos.
Siddhesh Poyarekar March 11, 2015, 3:44 p.m. UTC | #15
On Thu, Mar 05, 2015 at 03:05:20PM -0500, Carlos O'Donell wrote:
> On 03/02/2015 04:34 AM, Siddhesh Poyarekar wrote:
> > On Mon, Mar 02, 2015 at 10:29:31AM +0100, Andreas Schwab wrote:
> >> Siddhesh Poyarekar <siddhesh@redhat.com> writes:
> >>
> >>> Andreas, is this OK or are you still not convinced about adding this
> >>> patch in?
> >>
> >> How about fixing the real bug?
> > 
> > Carlos has posted a fix for nfs-utils[1].  The intent of including
> > this patch is not to work around that specific bug, but to make the
> > code robust against such bugs in future.
> > 
> > Siddhesh
> > 
> > [1] http://article.gmane.org/gmane.linux.nfs/69437
> > 
> 
> This should be committed to glibc.

Since Andreas' point of fixing the original bug has also been taken
care off, I'll commit this by the end of the week if there are no more
objections.

Siddhesh
Siddhesh Poyarekar March 18, 2015, 9:28 a.m. UTC | #16
On Wed, Mar 11, 2015 at 09:14:34PM +0530, Siddhesh Poyarekar wrote:
> Since Andreas' point of fixing the original bug has also been taken
> care off, I'll commit this by the end of the week if there are no more
> objections.

I have pushed this now.

Siddhesh
Carlos O'Donell March 18, 2015, 2:40 p.m. UTC | #17
On 03/18/2015 05:28 AM, Siddhesh Poyarekar wrote:
> On Wed, Mar 11, 2015 at 09:14:34PM +0530, Siddhesh Poyarekar wrote:
>> Since Andreas' point of fixing the original bug has also been taken
>> care off, I'll commit this by the end of the week if there are no more
>> objections.
> 
> I have pushed this now.

Thanks.

c.

Patch
diff mbox

diff --git a/sunrpc/svc.c b/sunrpc/svc.c
index ccf0902..30c3a93 100644
--- a/sunrpc/svc.c
+++ b/sunrpc/svc.c
@@ -97,8 +97,8 @@  xprt_register (SVCXPRT *xprt)
  
    if (xports == NULL)
      {
-      xports = (SVCXPRT **) malloc (_rpc_dtablesize () * sizeof (SVCXPRT *));
-      if (xports == NULL) /* Don´t add handle */
+      xports = (SVCXPRT **) calloc (_rpc_dtablesize (), sizeof (SVCXPRT *));
+      if (xports == NULL) /* Don't add handle */
  	return;
      }