IPC_INFO: Cast shmmax and shmall fields to uintptr_t [BZ #26736]

  On x32, the shmctl (IPC_INFO) syscall returns the x86-64 values for
shmmax and shmall.  Since x32 is limited to 32-bit address space,
shmmax and shmall should be casted to uintptr_t and shmmax should be
clamped to INT_MAX only if the size of shmmax is the size of int.
---
 sysdeps/unix/sysv/linux/shmctl.c            | 20 ++++++++++++++++----
 sysdeps/unix/sysv/linux/tst-sysvshm-linux.c |  3 ++-
 2 files changed, 18 insertions(+), 5 deletions(-)
  

On Thu, Oct 15, 2020 at 8:17 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On x32, the shmctl (IPC_INFO) syscall returns the x86-64 values for
> shmmax and shmall.  Since x32 is limited to 32-bit address space,
> shmmax and shmall should be casted to uintptr_t and shmmax should be
> clamped to INT_MAX only if the size of shmmax is the size of int.
> ---
>  sysdeps/unix/sysv/linux/shmctl.c            | 20 ++++++++++++++++----
>  sysdeps/unix/sysv/linux/tst-sysvshm-linux.c |  3 ++-
>  2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/shmctl.c b/sysdeps/unix/sysv/linux/shmctl.c
> index 833f013e69..8d34c0e7bf 100644
> --- a/sysdeps/unix/sysv/linux/shmctl.c
> +++ b/sysdeps/unix/sysv/linux/shmctl.c
> @@ -138,11 +138,11 @@ __shmctl64 (int shmid, int cmd, struct __shmid64_ds *buf)
>
>    switch (cmd)
>      {
> -      case IPC_STAT:
> -      case SHM_STAT:
> -      case SHM_STAT_ANY:
> +    case IPC_STAT:
> +    case SHM_STAT:
> +    case SHM_STAT_ANY:
>  #ifdef __ASSUME_SYSVIPC_BROKEN_MODE_T
> -        arg->shm_perm.mode >>= 16;
> +      arg->shm_perm.mode >>= 16;
>  #else
>        /* Old Linux kernel versions might not clear the mode padding.  */
>        if (sizeof ((struct shmid_ds){0}.shm_perm.mode)
> @@ -153,6 +153,18 @@ __shmctl64 (int shmid, int cmd, struct __shmid64_ds *buf)
>  #if __IPC_TIME64
>        kshmid64_to_shmid64 (arg, buf);
>  #endif
> +      break;
> +
> +    case IPC_INFO:
> +      if (sizeof (((struct shminfo *) 0)->shmmax) > sizeof (uintptr_t))
> +       {
> +         /* Since the address space is limited by the size of uintptr_t,
> +            cast shmmax and shmall to uintptr_t.  */
> +         struct shminfo *shminfo_p = (struct shminfo *) arg;
> +         shminfo_p->shmmax = (uintptr_t) shminfo_p->shmmax;
> +         shminfo_p->shmall = (uintptr_t) shminfo_p->shmall;
> +       }
> +      break;
>      }
>
>    return ret;
> diff --git a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> index 7128ae2e14..abeeb37a78 100644
> --- a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> +++ b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> @@ -128,7 +128,8 @@ do_test (void)
>  #if LONG_MAX == INT_MAX
>      /* Kernel explicit clamp the value for shmmax on compat symbol (32-bit
>         binaries running on 64-bit kernels).  */
> -    if (v > INT_MAX)
> +    if (sizeof (((struct shminfo *) 0)->shmmax) == sizeof (int)
> +       && v > INT_MAX)
>        v = INT_MAX;
>  #endif
>      tipcinfo.shmmax = v;
> --
> 2.26.2
>

Tested with scripts/build-many-glibcs.py and on i686/x32/x86-64.
  

On 15/10/2020 12:17, H.J. Lu via Libc-alpha wrote:
> On x32, the shmctl (IPC_INFO) syscall returns the x86-64 values for
> shmmax and shmall.  Since x32 is limited to 32-bit address space,
> shmmax and shmall should be casted to uintptr_t and shmmax should be
> clamped to INT_MAX only if the size of shmmax is the size of int.
> ---
>  sysdeps/unix/sysv/linux/shmctl.c            | 20 ++++++++++++++++----
>  sysdeps/unix/sysv/linux/tst-sysvshm-linux.c |  3 ++-
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/shmctl.c b/sysdeps/unix/sysv/linux/shmctl.c
> index 833f013e69..8d34c0e7bf 100644
> --- a/sysdeps/unix/sysv/linux/shmctl.c
> +++ b/sysdeps/unix/sysv/linux/shmctl.c
> @@ -138,11 +138,11 @@ __shmctl64 (int shmid, int cmd, struct __shmid64_ds *buf)
>  
>    switch (cmd)
>      {
> -      case IPC_STAT:
> -      case SHM_STAT:
> -      case SHM_STAT_ANY:
> +    case IPC_STAT:
> +    case SHM_STAT:
> +    case SHM_STAT_ANY:
>  #ifdef __ASSUME_SYSVIPC_BROKEN_MODE_T
> -        arg->shm_perm.mode >>= 16;
> +      arg->shm_perm.mode >>= 16;
>  #else
>        /* Old Linux kernel versions might not clear the mode padding.  */
>        if (sizeof ((struct shmid_ds){0}.shm_perm.mode)
> @@ -153,6 +153,18 @@ __shmctl64 (int shmid, int cmd, struct __shmid64_ds *buf)
>  #if __IPC_TIME64
>        kshmid64_to_shmid64 (arg, buf);
>  #endif
> +      break;
> +
> +    case IPC_INFO:
> +      if (sizeof (((struct shminfo *) 0)->shmmax) > sizeof (uintptr_t))
> +	{
> +	  /* Since the address space is limited by the size of uintptr_t,
> +	     cast shmmax and shmall to uintptr_t.  */
> +	  struct shminfo *shminfo_p = (struct shminfo *) arg;
> +	  shminfo_p->shmmax = (uintptr_t) shminfo_p->shmmax;
> +	  shminfo_p->shmall = (uintptr_t) shminfo_p->shmall;
> +	}
> +      break;
>      }
>  
>    return ret;
> diff --git a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> index 7128ae2e14..abeeb37a78 100644
> --- a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> +++ b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> @@ -128,7 +128,8 @@ do_test (void)
>  #if LONG_MAX == INT_MAX
>      /* Kernel explicit clamp the value for shmmax on compat symbol (32-bit
>         binaries running on 64-bit kernels).  */
> -    if (v > INT_MAX)
> +    if (sizeof (((struct shminfo *) 0)->shmmax) == sizeof (int)
> +	&& v > INT_MAX)
>        v = INT_MAX;
>  #endif
>      tipcinfo.shmmax = v;
> 

This is semantic change of shmctl for x32, which I really do not oppose
but I think we should rely on kernel to clamp the value correctly for x32
(the shmctl is already rather complex to handle y2038 safeness).

What about changing the testcase to use the expected types for shminfo
fields (__syscall_ulong_t instead of unsigned long int) and check for 
the expected provided kernel value:

---

diff --git a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
index 7128ae2e14..cb32bd522e 100644
--- a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
+++ b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
@@ -54,9 +54,9 @@ do_prepare (int argc, char *argv[])
 
 struct test_shminfo
 {
-  unsigned long int shmall;
-  unsigned long int shmmax;
-  unsigned long int shmmni;
+  __syscall_ulong_t shmall;
+  __syscall_ulong_t shmmax;
+  __syscall_ulong_t shmmni;
 };
 
 /* It tries to obtain some system-wide SysV shared memory information from
@@ -128,7 +128,8 @@ do_test (void)
 #if LONG_MAX == INT_MAX
     /* Kernel explicit clamp the value for shmmax on compat symbol (32-bit
        binaries running on 64-bit kernels).  */
-    if (v > INT_MAX)
+    if (sizeof (__syscall_ulong_t) == sizeof (unsigned long int)
+        && v > INT_MAX)
       v = INT_MAX;
 #endif
     tipcinfo.shmmax = v;
  

* H. J. Lu via Libc-alpha:

> diff --git a/sysdeps/unix/sysv/linux/shmctl.c b/sysdeps/unix/sysv/linux/shmctl.c
> index 833f013e69..8d34c0e7bf 100644
> --- a/sysdeps/unix/sysv/linux/shmctl.c
> +++ b/sysdeps/unix/sysv/linux/shmctl.c

> +    case IPC_INFO:
> +      if (sizeof (((struct shminfo *) 0)->shmmax) > sizeof (uintptr_t))

I think the C way of writing this is:

   sizeof ((struct shminfo) { 0 }.shmmax)

I believe a null pointer deference is still undefined in an
unevaluated sizeof context.

> diff --git a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> index 7128ae2e14..abeeb37a78 100644
> --- a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> +++ b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> @@ -128,7 +128,8 @@ do_test (void)
>  #if LONG_MAX == INT_MAX
>      /* Kernel explicit clamp the value for shmmax on compat symbol (32-bit
>         binaries running on 64-bit kernels).  */
> -    if (v > INT_MAX)
> +    if (sizeof (((struct shminfo *) 0)->shmmax) == sizeof (int)
> +	&& v > INT_MAX)

Likewise.
  

On Thu, Oct 15, 2020 at 10:24 AM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
>
> On 15/10/2020 12:17, H.J. Lu via Libc-alpha wrote:
> > On x32, the shmctl (IPC_INFO) syscall returns the x86-64 values for
> > shmmax and shmall.  Since x32 is limited to 32-bit address space,
> > shmmax and shmall should be casted to uintptr_t and shmmax should be
> > clamped to INT_MAX only if the size of shmmax is the size of int.
> > ---
> >  sysdeps/unix/sysv/linux/shmctl.c            | 20 ++++++++++++++++----
> >  sysdeps/unix/sysv/linux/tst-sysvshm-linux.c |  3 ++-
> >  2 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/sysdeps/unix/sysv/linux/shmctl.c b/sysdeps/unix/sysv/linux/shmctl.c
> > index 833f013e69..8d34c0e7bf 100644
> > --- a/sysdeps/unix/sysv/linux/shmctl.c
> > +++ b/sysdeps/unix/sysv/linux/shmctl.c
> > @@ -138,11 +138,11 @@ __shmctl64 (int shmid, int cmd, struct __shmid64_ds *buf)
> >
> >    switch (cmd)
> >      {
> > -      case IPC_STAT:
> > -      case SHM_STAT:
> > -      case SHM_STAT_ANY:
> > +    case IPC_STAT:
> > +    case SHM_STAT:
> > +    case SHM_STAT_ANY:
> >  #ifdef __ASSUME_SYSVIPC_BROKEN_MODE_T
> > -        arg->shm_perm.mode >>= 16;
> > +      arg->shm_perm.mode >>= 16;
> >  #else
> >        /* Old Linux kernel versions might not clear the mode padding.  */
> >        if (sizeof ((struct shmid_ds){0}.shm_perm.mode)
> > @@ -153,6 +153,18 @@ __shmctl64 (int shmid, int cmd, struct __shmid64_ds *buf)
> >  #if __IPC_TIME64
> >        kshmid64_to_shmid64 (arg, buf);
> >  #endif
> > +      break;
> > +
> > +    case IPC_INFO:
> > +      if (sizeof (((struct shminfo *) 0)->shmmax) > sizeof (uintptr_t))
> > +     {
> > +       /* Since the address space is limited by the size of uintptr_t,
> > +          cast shmmax and shmall to uintptr_t.  */
> > +       struct shminfo *shminfo_p = (struct shminfo *) arg;
> > +       shminfo_p->shmmax = (uintptr_t) shminfo_p->shmmax;
> > +       shminfo_p->shmall = (uintptr_t) shminfo_p->shmall;
> > +     }
> > +      break;
> >      }
> >
> >    return ret;
> > diff --git a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> > index 7128ae2e14..abeeb37a78 100644
> > --- a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> > +++ b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> > @@ -128,7 +128,8 @@ do_test (void)
> >  #if LONG_MAX == INT_MAX
> >      /* Kernel explicit clamp the value for shmmax on compat symbol (32-bit
> >         binaries running on 64-bit kernels).  */
> > -    if (v > INT_MAX)
> > +    if (sizeof (((struct shminfo *) 0)->shmmax) == sizeof (int)
> > +     && v > INT_MAX)
> >        v = INT_MAX;
> >  #endif
> >      tipcinfo.shmmax = v;
> >
>
> This is semantic change of shmctl for x32, which I really do not oppose
> but I think we should rely on kernel to clamp the value correctly for x32
> (the shmctl is already rather complex to handle y2038 safeness).
>
> What about changing the testcase to use the expected types for shminfo
> fields (__syscall_ulong_t instead of unsigned long int) and check for
> the expected provided kernel value:
>
> ---
>
> diff --git a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> index 7128ae2e14..cb32bd522e 100644
> --- a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> +++ b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
> @@ -54,9 +54,9 @@ do_prepare (int argc, char *argv[])
>
>  struct test_shminfo
>  {
> -  unsigned long int shmall;
> -  unsigned long int shmmax;
> -  unsigned long int shmmni;
> +  __syscall_ulong_t shmall;
> +  __syscall_ulong_t shmmax;
> +  __syscall_ulong_t shmmni;
>  };
>
>  /* It tries to obtain some system-wide SysV shared memory information from
> @@ -128,7 +128,8 @@ do_test (void)
>  #if LONG_MAX == INT_MAX
>      /* Kernel explicit clamp the value for shmmax on compat symbol (32-bit
>         binaries running on 64-bit kernels).  */
> -    if (v > INT_MAX)
> +    if (sizeof (__syscall_ulong_t) == sizeof (unsigned long int)
> +        && v > INT_MAX)
>        v = INT_MAX;
>  #endif
>      tipcinfo.shmmax = v;

This works.  Can you check it in?

Thanks.
  

On 15/10/2020 14:40, H.J. Lu wrote:
> On Thu, Oct 15, 2020 at 10:24 AM Adhemerval Zanella via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>>
>>
>> On 15/10/2020 12:17, H.J. Lu via Libc-alpha wrote:
>>> On x32, the shmctl (IPC_INFO) syscall returns the x86-64 values for
>>> shmmax and shmall.  Since x32 is limited to 32-bit address space,
>>> shmmax and shmall should be casted to uintptr_t and shmmax should be
>>> clamped to INT_MAX only if the size of shmmax is the size of int.
>>> ---
>>>  sysdeps/unix/sysv/linux/shmctl.c            | 20 ++++++++++++++++----
>>>  sysdeps/unix/sysv/linux/tst-sysvshm-linux.c |  3 ++-
>>>  2 files changed, 18 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/shmctl.c b/sysdeps/unix/sysv/linux/shmctl.c
>>> index 833f013e69..8d34c0e7bf 100644
>>> --- a/sysdeps/unix/sysv/linux/shmctl.c
>>> +++ b/sysdeps/unix/sysv/linux/shmctl.c
>>> @@ -138,11 +138,11 @@ __shmctl64 (int shmid, int cmd, struct __shmid64_ds *buf)
>>>
>>>    switch (cmd)
>>>      {
>>> -      case IPC_STAT:
>>> -      case SHM_STAT:
>>> -      case SHM_STAT_ANY:
>>> +    case IPC_STAT:
>>> +    case SHM_STAT:
>>> +    case SHM_STAT_ANY:
>>>  #ifdef __ASSUME_SYSVIPC_BROKEN_MODE_T
>>> -        arg->shm_perm.mode >>= 16;
>>> +      arg->shm_perm.mode >>= 16;
>>>  #else
>>>        /* Old Linux kernel versions might not clear the mode padding.  */
>>>        if (sizeof ((struct shmid_ds){0}.shm_perm.mode)
>>> @@ -153,6 +153,18 @@ __shmctl64 (int shmid, int cmd, struct __shmid64_ds *buf)
>>>  #if __IPC_TIME64
>>>        kshmid64_to_shmid64 (arg, buf);
>>>  #endif
>>> +      break;
>>> +
>>> +    case IPC_INFO:
>>> +      if (sizeof (((struct shminfo *) 0)->shmmax) > sizeof (uintptr_t))
>>> +     {
>>> +       /* Since the address space is limited by the size of uintptr_t,
>>> +          cast shmmax and shmall to uintptr_t.  */
>>> +       struct shminfo *shminfo_p = (struct shminfo *) arg;
>>> +       shminfo_p->shmmax = (uintptr_t) shminfo_p->shmmax;
>>> +       shminfo_p->shmall = (uintptr_t) shminfo_p->shmall;
>>> +     }
>>> +      break;
>>>      }
>>>
>>>    return ret;
>>> diff --git a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
>>> index 7128ae2e14..abeeb37a78 100644
>>> --- a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
>>> +++ b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
>>> @@ -128,7 +128,8 @@ do_test (void)
>>>  #if LONG_MAX == INT_MAX
>>>      /* Kernel explicit clamp the value for shmmax on compat symbol (32-bit
>>>         binaries running on 64-bit kernels).  */
>>> -    if (v > INT_MAX)
>>> +    if (sizeof (((struct shminfo *) 0)->shmmax) == sizeof (int)
>>> +     && v > INT_MAX)
>>>        v = INT_MAX;
>>>  #endif
>>>      tipcinfo.shmmax = v;
>>>
>>
>> This is semantic change of shmctl for x32, which I really do not oppose
>> but I think we should rely on kernel to clamp the value correctly for x32
>> (the shmctl is already rather complex to handle y2038 safeness).
>>
>> What about changing the testcase to use the expected types for shminfo
>> fields (__syscall_ulong_t instead of unsigned long int) and check for
>> the expected provided kernel value:
>>
>> ---
>>
>> diff --git a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
>> index 7128ae2e14..cb32bd522e 100644
>> --- a/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
>> +++ b/sysdeps/unix/sysv/linux/tst-sysvshm-linux.c
>> @@ -54,9 +54,9 @@ do_prepare (int argc, char *argv[])
>>
>>  struct test_shminfo
>>  {
>> -  unsigned long int shmall;
>> -  unsigned long int shmmax;
>> -  unsigned long int shmmni;
>> +  __syscall_ulong_t shmall;
>> +  __syscall_ulong_t shmmax;
>> +  __syscall_ulong_t shmmni;
>>  };
>>
>>  /* It tries to obtain some system-wide SysV shared memory information from
>> @@ -128,7 +128,8 @@ do_test (void)
>>  #if LONG_MAX == INT_MAX
>>      /* Kernel explicit clamp the value for shmmax on compat symbol (32-bit
>>         binaries running on 64-bit kernels).  */
>> -    if (v > INT_MAX)
>> +    if (sizeof (__syscall_ulong_t) == sizeof (unsigned long int)
>> +        && v > INT_MAX)
>>        v = INT_MAX;
>>  #endif
>>      tipcinfo.shmmax = v;
> 
> This works.  Can you check it in?

I will do it.