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

[Xiaoming Ni]

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