[PATCH] Handle out-of-memory case in svc_tcp.c/svc_unix.c:rendezvous_request.

[PATCH] Handle out-of-memory case in svc_tcp.c/svc_unix.c:rendezvous_request.

[Stefan Liebler]

If glibc is build with -O3 on at least 390 (-m31) or x86 (-m32),
gcc 11 dumps this warning:
svc_tcp.c: In function 'rendezvous_request':
svc_tcp.c:274:3: error: 'memcpy' offset [0, 15] is out of the bounds [0, 0] [-Werror=array-bounds]
  274 |   memcpy (&xprt->xp_raddr, &addr, sizeof (addr));
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

In out-of-memory case, if one of the mallocs in makefd_xprt function
returns NULL, a message is dumped, makefd_xprt returns NULL
and the subsequent memcpy would copy to NULL.

Instead of a segfaulting, svctcp_rendezvous_abort is now called.

The same applies to svc_unix.c.
---
 sunrpc/svc_tcp.c  | 5 +++++
 sunrpc/svc_unix.c | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/sunrpc/svc_tcp.c b/sunrpc/svc_tcp.c
index efbdd22548..738d47edb0 100644
--- a/sunrpc/svc_tcp.c
+++ b/sunrpc/svc_tcp.c
@@ -271,6 +271,11 @@ again:
    * make a new transporter (re-uses xprt)
    */
   xprt = makefd_xprt (sock, r->sendsize, r->recvsize);
+
+  /* If we are out of memory, makefd_xprt has already dumped an error.  */
+  if (xprt == NULL)
+    svctcp_rendezvous_abort ();
+
   memcpy (&xprt->xp_raddr, &addr, sizeof (addr));
   xprt->xp_addrlen = len;
   return FALSE;		/* there is never an rpc msg to be processed */
diff --git a/sunrpc/svc_unix.c b/sunrpc/svc_unix.c
index e01afeabe6..b13a4cd282 100644
--- a/sunrpc/svc_unix.c
+++ b/sunrpc/svc_unix.c
@@ -270,6 +270,11 @@ again:
   memset (&in_addr, '\0', sizeof (in_addr));
   in_addr.sin_family = AF_UNIX;
   xprt = makefd_xprt (sock, r->sendsize, r->recvsize);
+
+  /* If we are out of memory, makefd_xprt has already dumped an error.  */
+  if (xprt == NULL)
+    svcunix_rendezvous_abort ();
+
   memcpy (&xprt->xp_raddr, &in_addr, sizeof (in_addr));
   xprt->xp_addrlen = len;
   return FALSE;		/* there is never an rpc msg to be processed */
-- 
2.23.0

Re: [PATCH] Handle out-of-memory case in svc_tcp.c/svc_unix.c:rendezvous_request.

[Adhemerval Zanella]

On 02/12/2020 05:56, Stefan Liebler via Libc-alpha wrote:
> If glibc is build with -O3 on at least 390 (-m31) or x86 (-m32),
> gcc 11 dumps this warning:
> svc_tcp.c: In function 'rendezvous_request':
> svc_tcp.c:274:3: error: 'memcpy' offset [0, 15] is out of the bounds [0, 0] [-Werror=array-bounds]
>   274 |   memcpy (&xprt->xp_raddr, &addr, sizeof (addr));
>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> In out-of-memory case, if one of the mallocs in makefd_xprt function
> returns NULL, a message is dumped, makefd_xprt returns NULL
> and the subsequent memcpy would copy to NULL.
> 
> Instead of a segfaulting, svctcp_rendezvous_abort is now called.

It does not do what other parts of sunrpc does in case of memory allocation
failure, it seems that usually the idea is to do some cleanup and return
FALSE (for the case if the function returns bool_t).

> 
> The same applies to svc_unix.c.
> ---
>  sunrpc/svc_tcp.c  | 5 +++++
>  sunrpc/svc_unix.c | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/sunrpc/svc_tcp.c b/sunrpc/svc_tcp.c
> index efbdd22548..738d47edb0 100644
> --- a/sunrpc/svc_tcp.c
> +++ b/sunrpc/svc_tcp.c
> @@ -271,6 +271,11 @@ again:
>     * make a new transporter (re-uses xprt)
>     */
>    xprt = makefd_xprt (sock, r->sendsize, r->recvsize);
> +
> +  /* If we are out of memory, makefd_xprt has already dumped an error.  */
> +  if (xprt == NULL)
> +    svctcp_rendezvous_abort ();
> +
>    memcpy (&xprt->xp_raddr, &addr, sizeof (addr));
>    xprt->xp_addrlen = len;
>    return FALSE;		/* there is never an rpc msg to be processed */
> diff --git a/sunrpc/svc_unix.c b/sunrpc/svc_unix.c
> index e01afeabe6..b13a4cd282 100644
> --- a/sunrpc/svc_unix.c
> +++ b/sunrpc/svc_unix.c
> @@ -270,6 +270,11 @@ again:
>    memset (&in_addr, '\0', sizeof (in_addr));
>    in_addr.sin_family = AF_UNIX;
>    xprt = makefd_xprt (sock, r->sendsize, r->recvsize);
> +
> +  /* If we are out of memory, makefd_xprt has already dumped an error.  */
> +  if (xprt == NULL)
> +    svcunix_rendezvous_abort ();
> +
>    memcpy (&xprt->xp_raddr, &in_addr, sizeof (in_addr));
>    xprt->xp_addrlen = len;
>    return FALSE;		/* there is never an rpc msg to be processed */
> 

Re: [PATCH] Handle out-of-memory case in svc_tcp.c/svc_unix.c:rendezvous_request.

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> On 02/12/2020 05:56, Stefan Liebler via Libc-alpha wrote:
>> If glibc is build with -O3 on at least 390 (-m31) or x86 (-m32),
>> gcc 11 dumps this warning:
>> svc_tcp.c: In function 'rendezvous_request':
>> svc_tcp.c:274:3: error: 'memcpy' offset [0, 15] is out of the bounds [0, 0] [-Werror=array-bounds]
>>   274 |   memcpy (&xprt->xp_raddr, &addr, sizeof (addr));
>>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> cc1: all warnings being treated as errors
>> 
>> In out-of-memory case, if one of the mallocs in makefd_xprt function
>> returns NULL, a message is dumped, makefd_xprt returns NULL
>> and the subsequent memcpy would copy to NULL.
>> 
>> Instead of a segfaulting, svctcp_rendezvous_abort is now called.
>
> It does not do what other parts of sunrpc does in case of memory allocation
> failure, it seems that usually the idea is to do some cleanup and return
> FALSE (for the case if the function returns bool_t).

I think returning FALSE would introduce a variant of CVE-2011-4609 (bug
14889).  The sleeping added in commit 14bc93a967e62abf8cf2704725b6f7 for
that is rather hackish (more correct would be to remove the accepting
socket from the polling set until a client exits), but sleeping on
ENOMEM might be a reasonable approach here, given that the sunrpc code
is deep maintenance.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH] Handle out-of-memory case in svc_tcp.c/svc_unix.c:rendezvous_request.

[Adhemerval Zanella]

On 03/12/2020 18:03, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> On 02/12/2020 05:56, Stefan Liebler via Libc-alpha wrote:
>>> If glibc is build with -O3 on at least 390 (-m31) or x86 (-m32),
>>> gcc 11 dumps this warning:
>>> svc_tcp.c: In function 'rendezvous_request':
>>> svc_tcp.c:274:3: error: 'memcpy' offset [0, 15] is out of the bounds [0, 0] [-Werror=array-bounds]
>>>   274 |   memcpy (&xprt->xp_raddr, &addr, sizeof (addr));
>>>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> cc1: all warnings being treated as errors
>>>
>>> In out-of-memory case, if one of the mallocs in makefd_xprt function
>>> returns NULL, a message is dumped, makefd_xprt returns NULL
>>> and the subsequent memcpy would copy to NULL.
>>>
>>> Instead of a segfaulting, svctcp_rendezvous_abort is now called.
>>
>> It does not do what other parts of sunrpc does in case of memory allocation
>> failure, it seems that usually the idea is to do some cleanup and return
>> FALSE (for the case if the function returns bool_t).
> 
> I think returning FALSE would introduce a variant of CVE-2011-4609 (bug
> 14889).  The sleeping added in commit 14bc93a967e62abf8cf2704725b6f7 for
> that is rather hackish (more correct would be to remove the accepting
> socket from the polling set until a client exits), but sleeping on
> ENOMEM might be a reasonable approach here, given that the sunrpc code
> is deep maintenance.

Using the sleep hack should be slight better and avoid ENOMEM to abort
the process.

Re: [PATCH] Handle out-of-memory case in svc_tcp.c/svc_unix.c:rendezvous_request.

[Stefan Liebler]

On 12/4/20 1:25 PM, Adhemerval Zanella via Libc-alpha wrote:
> 
> 
> On 03/12/2020 18:03, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> On 02/12/2020 05:56, Stefan Liebler via Libc-alpha wrote:
>>>> If glibc is build with -O3 on at least 390 (-m31) or x86 (-m32),
>>>> gcc 11 dumps this warning:
>>>> svc_tcp.c: In function 'rendezvous_request':
>>>> svc_tcp.c:274:3: error: 'memcpy' offset [0, 15] is out of the bounds [0, 0] [-Werror=array-bounds]
>>>>   274 |   memcpy (&xprt->xp_raddr, &addr, sizeof (addr));
>>>>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> cc1: all warnings being treated as errors
>>>>
>>>> In out-of-memory case, if one of the mallocs in makefd_xprt function
>>>> returns NULL, a message is dumped, makefd_xprt returns NULL
>>>> and the subsequent memcpy would copy to NULL.
>>>>
>>>> Instead of a segfaulting, svctcp_rendezvous_abort is now called.
>>>
>>> It does not do what other parts of sunrpc does in case of memory allocation
>>> failure, it seems that usually the idea is to do some cleanup and return
>>> FALSE (for the case if the function returns bool_t).
>>
>> I think returning FALSE would introduce a variant of CVE-2011-4609 (bug
>> 14889).  The sleeping added in commit 14bc93a967e62abf8cf2704725b6f7 for
>> that is rather hackish (more correct would be to remove the accepting
>> socket from the polling set until a client exits), but sleeping on
>> ENOMEM might be a reasonable approach here, given that the sunrpc code
>> is deep maintenance.
> 
> Using the sleep hack should be slight better and avoid ENOMEM to abort
> the process.
> 

Now it sleeps and FALSE is returned.
Please have a look at:
"[PATCH v2] Handle out-of-memory case in
svc_tcp.c/svc_unix.c:rendezvous_request."
https://sourceware.org/pipermail/libc-alpha/2020-December/120410.html

Thanks,
Stefan