diff mbox series

grantpt: Get rid of alloca

Message ID 20230601195817.3251303-1-josimmon@redhat.com
State Superseded
Headers
Series grantpt: Get rid of alloca |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 pending Patch applied
linaro-tcwg-bot/tcwg_glibc_check--master-arm pending Patch applied

Commit Message

Joe Simmons-Talbott June 1, 2023, 7:58 p.m. UTC 
  Replace alloca with a scratch_buffer to avoid potential stack overflows.
---
 sysdeps/unix/grantpt.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Florian Weimer June 5, 2023, 1:29 p.m. UTC | #1 
* Joe Simmons-Talbott via Libc-alpha:

> Replace alloca with a scratch_buffer to avoid potential stack overflows.
> ---
>  sysdeps/unix/grantpt.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
> index 38fce52576..e5d2390bf2 100644
> --- a/sysdeps/unix/grantpt.c
> +++ b/sysdeps/unix/grantpt.c
> @@ -20,6 +20,7 @@
>  #include <fcntl.h>
>  #include <grp.h>
>  #include <limits.h>
> +#include <scratch_buffer.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <sys/resource.h>
> @@ -147,7 +148,14 @@ grantpt (int fd)
>  	/* `sysconf' does not support _SC_GETGR_R_SIZE_MAX.
>  	   Try a moderate value.  */
>  	grbuflen = 1024;
> -      grtmpbuf = (char *) __alloca (grbuflen);
> +      struct scratch_buffer sbuf;
> +      scratch_buffer_init (&sbuf);
> +      if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen))
> +	{
> +	  retval -1;
> +	  goto cleanup;
> +	}
> +      grtmpbuf = sbuf.data;
>        __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p);
>        if (p != NULL)
>  	tty_gid = p->gr_gid;
> @@ -255,6 +263,8 @@ grantpt (int fd)
>    if (buf != _buf)
>      free (buf);
>  
> +  scratch_buffer_free(sbuf);
> +
>    return retval;
>  }
>  libc_hidden_def (grantpt)

How did you test this?  This code is only used on Hurd due to the
override in sysdeps/unix/sysv/linux/grantpt.c.

Maybe the hurd maintainers could review this change?

Thanks,
Florian
Joe Simmons-Talbott June 5, 2023, 1:37 p.m. UTC | #2 
On Mon, Jun 05, 2023 at 03:29:45PM +0200, Florian Weimer wrote:
> * Joe Simmons-Talbott via Libc-alpha:
> 
> > Replace alloca with a scratch_buffer to avoid potential stack overflows.
> > ---
> >  sysdeps/unix/grantpt.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
> > index 38fce52576..e5d2390bf2 100644
> > --- a/sysdeps/unix/grantpt.c
> > +++ b/sysdeps/unix/grantpt.c
> > @@ -20,6 +20,7 @@
> >  #include <fcntl.h>
> >  #include <grp.h>
> >  #include <limits.h>
> > +#include <scratch_buffer.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> >  #include <sys/resource.h>
> > @@ -147,7 +148,14 @@ grantpt (int fd)
> >  	/* `sysconf' does not support _SC_GETGR_R_SIZE_MAX.
> >  	   Try a moderate value.  */
> >  	grbuflen = 1024;
> > -      grtmpbuf = (char *) __alloca (grbuflen);
> > +      struct scratch_buffer sbuf;
> > +      scratch_buffer_init (&sbuf);
> > +      if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen))
> > +	{
> > +	  retval -1;
> > +	  goto cleanup;
> > +	}
> > +      grtmpbuf = sbuf.data;
> >        __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p);
> >        if (p != NULL)
> >  	tty_gid = p->gr_gid;
> > @@ -255,6 +263,8 @@ grantpt (int fd)
> >    if (buf != _buf)
> >      free (buf);
> >  
> > +  scratch_buffer_free(sbuf);
> > +
> >    return retval;
> >  }
> >  libc_hidden_def (grantpt)
> 
> How did you test this?  This code is only used on Hurd due to the
> override in sysdeps/unix/sysv/linux/grantpt.c.

The only testing I did was a 'make && make check' on x86_64 and i686 linux.

Thanks,
Joe
> 
> Maybe the hurd maintainers could review this change?
> 
> Thanks,
> Florian
>
Adhemerval Zanella June 5, 2023, 6:14 p.m. UTC | #3 
On 05/06/23 10:37, Joe Simmons-Talbott via Libc-alpha wrote:
> On Mon, Jun 05, 2023 at 03:29:45PM +0200, Florian Weimer wrote:
>> * Joe Simmons-Talbott via Libc-alpha:
>>
>>> Replace alloca with a scratch_buffer to avoid potential stack overflows.
>>> ---
>>>  sysdeps/unix/grantpt.c | 12 +++++++++++-
>>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
>>> index 38fce52576..e5d2390bf2 100644
>>> --- a/sysdeps/unix/grantpt.c
>>> +++ b/sysdeps/unix/grantpt.c
>>> @@ -20,6 +20,7 @@
>>>  #include <fcntl.h>
>>>  #include <grp.h>
>>>  #include <limits.h>
>>> +#include <scratch_buffer.h>
>>>  #include <stdlib.h>
>>>  #include <string.h>
>>>  #include <sys/resource.h>
>>> @@ -147,7 +148,14 @@ grantpt (int fd)
>>>  	/* `sysconf' does not support _SC_GETGR_R_SIZE_MAX.
>>>  	   Try a moderate value.  */
>>>  	grbuflen = 1024;
>>> -      grtmpbuf = (char *) __alloca (grbuflen);
>>> +      struct scratch_buffer sbuf;
>>> +      scratch_buffer_init (&sbuf);
>>> +      if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen))
>>> +	{
>>> +	  retval -1;
>>> +	  goto cleanup;
>>> +	}
>>> +      grtmpbuf = sbuf.data;
>>>        __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p);
>>>        if (p != NULL)
>>>  	tty_gid = p->gr_gid;
>>> @@ -255,6 +263,8 @@ grantpt (int fd)
>>>    if (buf != _buf)
>>>      free (buf);
>>>  
>>> +  scratch_buffer_free(sbuf);
>>> +
>>>    return retval;
>>>  }
>>>  libc_hidden_def (grantpt)
>>
>> How did you test this?  This code is only used on Hurd due to the
>> override in sysdeps/unix/sysv/linux/grantpt.c.
> 
> The only testing I did was a 'make && make check' on x86_64 and i686 linux.

You will need to build for i686-gnu (Hurd) to actually enable this code.  Which
leads to another, which Siddheash has brough, which is --enable-pt_chown does
make sense to be provided as a configure switch.

I think we should just move all pt_chown to be Hurd specific, and remove the
--enable-pt_chown option.
Joe Simmons-Talbott June 5, 2023, 10:02 p.m. UTC | #4 
On Mon, Jun 05, 2023 at 03:14:06PM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 05/06/23 10:37, Joe Simmons-Talbott via Libc-alpha wrote:
> > On Mon, Jun 05, 2023 at 03:29:45PM +0200, Florian Weimer wrote:
> >> * Joe Simmons-Talbott via Libc-alpha:
> >>
> >>> Replace alloca with a scratch_buffer to avoid potential stack overflows.
> >>> ---
> >>>  sysdeps/unix/grantpt.c | 12 +++++++++++-
> >>>  1 file changed, 11 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
> >>> index 38fce52576..e5d2390bf2 100644
> >>> --- a/sysdeps/unix/grantpt.c
> >>> +++ b/sysdeps/unix/grantpt.c
> >>> @@ -20,6 +20,7 @@
> >>>  #include <fcntl.h>
> >>>  #include <grp.h>
> >>>  #include <limits.h>
> >>> +#include <scratch_buffer.h>
> >>>  #include <stdlib.h>
> >>>  #include <string.h>
> >>>  #include <sys/resource.h>
> >>> @@ -147,7 +148,14 @@ grantpt (int fd)
> >>>  	/* `sysconf' does not support _SC_GETGR_R_SIZE_MAX.
> >>>  	   Try a moderate value.  */
> >>>  	grbuflen = 1024;
> >>> -      grtmpbuf = (char *) __alloca (grbuflen);
> >>> +      struct scratch_buffer sbuf;
> >>> +      scratch_buffer_init (&sbuf);
> >>> +      if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen))
> >>> +	{
> >>> +	  retval -1;
> >>> +	  goto cleanup;
> >>> +	}
> >>> +      grtmpbuf = sbuf.data;
> >>>        __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p);
> >>>        if (p != NULL)
> >>>  	tty_gid = p->gr_gid;
> >>> @@ -255,6 +263,8 @@ grantpt (int fd)
> >>>    if (buf != _buf)
> >>>      free (buf);
> >>>  
> >>> +  scratch_buffer_free(sbuf);
> >>> +
> >>>    return retval;
> >>>  }
> >>>  libc_hidden_def (grantpt)
> >>
> >> How did you test this?  This code is only used on Hurd due to the
> >> override in sysdeps/unix/sysv/linux/grantpt.c.
> > 
> > The only testing I did was a 'make && make check' on x86_64 and i686 linux.
> 
> You will need to build for i686-gnu (Hurd) to actually enable this code.  Which
> leads to another, which Siddheash has brough, which is --enable-pt_chown does
> make sense to be provided as a configure switch.

I tested this patch today with build-many-glibcs.py for i686-gnu and it
passed 'make check'.  Does your second sentence mean that I needed to
pass '--enable-pt_chown' to configure for the test build?

Thanks,
Joe
Siddhesh Poyarekar June 6, 2023, 6:14 a.m. UTC | #5 
On 2023-06-05 18:02, Joe Simmons-Talbott wrote:
> On Mon, Jun 05, 2023 at 03:14:06PM -0300, Adhemerval Zanella Netto wrote:
>>
>>
>> On 05/06/23 10:37, Joe Simmons-Talbott via Libc-alpha wrote:
>>> On Mon, Jun 05, 2023 at 03:29:45PM +0200, Florian Weimer wrote:
>>>> * Joe Simmons-Talbott via Libc-alpha:
>>>>
>>>>> Replace alloca with a scratch_buffer to avoid potential stack overflows.
>>>>> ---
>>>>>   sysdeps/unix/grantpt.c | 12 +++++++++++-
>>>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
>>>>> index 38fce52576..e5d2390bf2 100644
>>>>> --- a/sysdeps/unix/grantpt.c
>>>>> +++ b/sysdeps/unix/grantpt.c
>>>>> @@ -20,6 +20,7 @@
>>>>>   #include <fcntl.h>
>>>>>   #include <grp.h>
>>>>>   #include <limits.h>
>>>>> +#include <scratch_buffer.h>
>>>>>   #include <stdlib.h>
>>>>>   #include <string.h>
>>>>>   #include <sys/resource.h>
>>>>> @@ -147,7 +148,14 @@ grantpt (int fd)
>>>>>   	/* `sysconf' does not support _SC_GETGR_R_SIZE_MAX.
>>>>>   	   Try a moderate value.  */
>>>>>   	grbuflen = 1024;
>>>>> -      grtmpbuf = (char *) __alloca (grbuflen);
>>>>> +      struct scratch_buffer sbuf;
>>>>> +      scratch_buffer_init (&sbuf);
>>>>> +      if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen))
>>>>> +	{
>>>>> +	  retval -1;
>>>>> +	  goto cleanup;
>>>>> +	}
>>>>> +      grtmpbuf = sbuf.data;
>>>>>         __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p);
>>>>>         if (p != NULL)
>>>>>   	tty_gid = p->gr_gid;
>>>>> @@ -255,6 +263,8 @@ grantpt (int fd)
>>>>>     if (buf != _buf)
>>>>>       free (buf);
>>>>>   
>>>>> +  scratch_buffer_free(sbuf);
>>>>> +
>>>>>     return retval;
>>>>>   }
>>>>>   libc_hidden_def (grantpt)
>>>>
>>>> How did you test this?  This code is only used on Hurd due to the
>>>> override in sysdeps/unix/sysv/linux/grantpt.c.
>>>
>>> The only testing I did was a 'make && make check' on x86_64 and i686 linux.
>>
>> You will need to build for i686-gnu (Hurd) to actually enable this code.  Which
>> leads to another, which Siddheash has brough, which is --enable-pt_chown does
>> make sense to be provided as a configure switch.

Yeah I looked at the code again and perhaps it's something for Samuel to 
think about.  We don't seem to have target-specific configure options, 
so it's a nop for Linux.

> I tested this patch today with build-many-glibcs.py for i686-gnu and it
> passed 'make check'.  Does your second sentence mean that I needed to
> pass '--enable-pt_chown' to configure for the test build?

Not really, the code you're changing is outside of HAVE_PT_CHOWN, so 
you're good to go I think.  I'd wait for Samuel to ack this since this 
is exclusively hurd code.

Thanks,
Sid
Samuel Thibault June 7, 2023, 12:40 a.m. UTC | #6 
Hello,

* Joe Simmons-Talbott via Libc-alpha:
> > Replace alloca with a scratch_buffer to avoid potential stack overflows.
> > ---
> >  sysdeps/unix/grantpt.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
> > index 38fce52576..e5d2390bf2 100644
> > --- a/sysdeps/unix/grantpt.c
> > +++ b/sysdeps/unix/grantpt.c
> > @@ -147,7 +148,14 @@ grantpt (int fd)
> >  	/* `sysconf' does not support _SC_GETGR_R_SIZE_MAX.
> >  	   Try a moderate value.  */
> >  	grbuflen = 1024;
> > -      grtmpbuf = (char *) __alloca (grbuflen);
> > +      struct scratch_buffer sbuf;
> > +      scratch_buffer_init (&sbuf);
> > +      if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen))
> > +	{
> > +	  retval -1;

This does not compile, missing =

> > +	  goto cleanup;
> > +	}
> > +      grtmpbuf = sbuf.data;
> >        __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p);
> >        if (p != NULL)
> >  	tty_gid = p->gr_gid;
> > @@ -255,6 +263,8 @@ grantpt (int fd)
> >    if (buf != _buf)
> >      free (buf);
> >  
> > +  scratch_buffer_free(sbuf);

sbuf is undefined here since it was defined in the 

if (__glibc_unlikely (tty_gid == -1))

block. Probably you just want to just move the scratch_buffer_free call
to the end of that block. Also, you need to pass &sbuf, not sbuf.

Samuel
Zack Weinberg June 7, 2023, 7:05 p.m. UTC | #7 
On Tue, Jun 6, 2023, at 2:14 AM, Siddhesh Poyarekar wrote:
...
>>>>> How did you test this?  This code is only used on Hurd due to the
>>>>> override in sysdeps/unix/sysv/linux/grantpt.c.
>>>>
>>>> The only testing I did was a 'make && make check' on x86_64 and
>>>> i686 linux.
>>>
>>> You will need to build for i686-gnu (Hurd) to actually enable this
>>> code.  Which leads to another, which Siddheash has brough, which is
>>> --enable-pt_chown does make sense to be provided as a configure
>>> switch.
>
> Yeah I looked at the code again and perhaps it's something for Samuel
> to think about.  We don't seem to have target-specific configure
> options, so it's a nop for Linux.

pt_chown has been troublesome in the past (up to and including privilege
escalation vulnerabilities) and, iirc, it's the only helper binary we
build that needs to be setuid.  It would be nice if we could get rid of
it altogether.  I believe the requirements for promoting the Linux-
specific code to generic (or just all-GNU) are for the Hurd to implement
a /dev/pts virtual file system that automatically adjusts the ownership
and permissions of /dev/pts/NNN when someone opens /dev/ptmx, and
additionally for it to add support for whichever of the TIOCSPTLCK,
TIOCGPTLCK, and TIOCGPTPEER ioctls are not already implemented.

zw
diff mbox series

Patch

diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
index 38fce52576..e5d2390bf2 100644
--- a/sysdeps/unix/grantpt.c
+++ b/sysdeps/unix/grantpt.c
@@ -20,6 +20,7 @@ 
 #include <fcntl.h>
 #include <grp.h>
 #include <limits.h>
+#include <scratch_buffer.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/resource.h>
@@ -147,7 +148,14 @@  grantpt (int fd)
 	/* `sysconf' does not support _SC_GETGR_R_SIZE_MAX.
 	   Try a moderate value.  */
 	grbuflen = 1024;
-      grtmpbuf = (char *) __alloca (grbuflen);
+      struct scratch_buffer sbuf;
+      scratch_buffer_init (&sbuf);
+      if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen))
+	{
+	  retval -1;
+	  goto cleanup;
+	}
+      grtmpbuf = sbuf.data;
       __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p);
       if (p != NULL)
 	tty_gid = p->gr_gid;
@@ -255,6 +263,8 @@  grantpt (int fd)
   if (buf != _buf)
     free (buf);
 
+  scratch_buffer_free(sbuf);
+
   return retval;
 }
 libc_hidden_def (grantpt)