[PATCH] setrlimit/getrlimit: Add parameter check to prevent null pointer access

[Adhemerval Zanella]

On 31/12/2020 01:01, Xiaoming Ni wrote:
> On 2020/12/31 10:44, Xiaoming Ni wrote:
>> On 2020/12/30 20:43, Adhemerval Zanella wrote:
>>>
>>>
>>> On 30/12/2020 08:41, Xiaoming Ni wrote:
>>>> Following sysdeps/mach/hurd/[gs]etrlimit.c.
>>>> Add parameter check to prevent null pointer access in setrlimit().
>>>> Using Macro Functions RETURN_IF_RLIMIT_EINVAL() to Avoid Duplicate Code Copy.
>>>
>>> POSIX [1] does not define that EINVAL should be returned for invalid
>>> argument, only for an invalid resource or if limit specified cannot
>>> be lowered.
>>>
>>> LTP used to check for a *Linux* specific semantic where it returned
>>> EFAULT for such cases; but it was a wrong assumption since getrlimit
>>> could be reimplemented through another syscall (such as currently
>>> on glibc it calls prlimit).
>>>
>>> I think we should mark the argument nonull instead and remove this
>>> check from Hurd (it will need to check againt RLIMIT_NLIMITS
>>> since it access a internal array).
>>>
>>> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/getrlimit.html
>>>
>>
>> Is that so?
>>
>> diff --git a/resource/setrlimit64.c b/resource/setrlimit64.c
>> index 0411e3ea19..c73ce59139 100644
>> --- a/resource/setrlimit64.c
>> +++ b/resource/setrlimit64.c
>> @@ -27,6 +27,8 @@ setrlimit64 (enum __rlimit_resource resource, const struct rlimit64 *rlimits)
>>   {
>>     struct rlimit rlimits32;
>>
>> +  RETURN_IF_RLIMITS_IS_NULL(rlimits);
>> +
>>     if (rlimits->rlim_cur >= RLIM_INFINITY)
>>       rlimits32.rlim_cur = RLIM_INFINITY;
>>     else
>> diff --git a/resource/sys/resource.h b/resource/sys/resource.h
>> index 4edafb50d5..c95ef35bfc 100644
>> --- a/resource/sys/resource.h
>> +++ b/resource/sys/resource.h
>> @@ -82,6 +82,14 @@ extern int setrlimit64 (__rlimit_resource_t __resource,
>>                          const struct rlimit64 *__rlimits) __THROW;
>>   #endif
>>
>> +#define RETURN_IF_RLIMITS_IS_NULL(rlimits) do { \
>> +  if ((rlimits) == NULL) \
>> +    { \
>> +      errno = EFAULT; \
>> +      return -1; \
>> +    } \
>> +} while(0)
>> +
>>   /* Return resource usage information on process indicated by WHO
>>      and put it in *USAGE.  Returns 0 for success, -1 for failure.  */
>>   extern int getrusage (__rusage_who_t __who, struct rusage *__usage) __THROW;
>> diff --git a/sysdeps/mach/hurd/getrlimit.c b/sysdeps/mach/hurd/getrlimit.c
>> index 32d37c185d..3ffcf80fc3 100644
>> --- a/sysdeps/mach/hurd/getrlimit.c
>> +++ b/sysdeps/mach/hurd/getrlimit.c
>> @@ -27,7 +27,8 @@ __getrlimit (enum __rlimit_resource resource, struct rlimit *rlimits)
>>   {
>>     struct rlimit lim;
>>
>> -  if (rlimits == NULL || (unsigned int) resource >= RLIMIT_NLIMITS)
>> +  RETURN_IF_RLIMITS_IS_NULL(rlimits);
>> +  if ((unsigned int) (resource) >= RLIMIT_NLIMITS)
>>       {
>>         errno = EINVAL;
>>         return -1;
>>
>>
>>
> 
> or this?
> 
> --- a/resource/sys/resource.h
> +++ b/resource/sys/resource.h
> @@ -67,7 +67,8 @@ extern int getrlimit64 (__rlimit_resource_t __resource,
>     Return 0 if successful, -1 if not (and sets errno).  */
>  #ifndef __USE_FILE_OFFSET64
>  extern int setrlimit (__rlimit_resource_t __resource,
> -                 const struct rlimit *__rlimits) __THROW;
> +               const struct rlimit *__rlimits)
> +                  __THROW __nonnull ((2));;
>  #else
> 
> 

Yes, you might also add the attribute on getrlimit and also on prlimit.