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

[Xiaoming Ni]

On 2020/12/31 18:52, Adhemerval Zanella wrote:
> 
> 
> 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.
> .
> 
__nonnull((2)) can only check null pointers in the compilation phase,
As an external interface, the getrlimit()/setrlimit()/prlimit() needs to 
check the null pointer during the running of the code to prevent the 
system from crashing.
so, Is it better to use both RETURN_IF_RLIMITS_IS_NULL() and __nonnull()?

Thanks
Xiaoming NI