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

[Xiaoming Ni]

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;