[PATCH] grantpt: Get rid of alloca

[PATCH] grantpt: Get rid of alloca

[Joe Simmons-Talbott]

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)
-- 
2.39.2

Re: [PATCH] grantpt: Get rid of alloca

[Florian Weimer]

* 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

Re: [PATCH] grantpt: Get rid of alloca

[Joe Simmons-Talbott]

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
> 

Re: [PATCH] grantpt: Get rid of alloca

[Adhemerval Zanella Netto]

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.

Re: [PATCH] grantpt: Get rid of alloca

[Joe Simmons-Talbott]

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

Re: [PATCH] grantpt: Get rid of alloca

[Siddhesh Poyarekar]

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

Re: [PATCH] grantpt: Get rid of alloca

[Zack Weinberg]

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

Re: [PATCH] grantpt: Get rid of alloca

[Samuel Thibault]

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