Re: [Valgrind-developers] [valgrind] PR504341: Prevent LTP setrlimit05 syscall test from crashing valgrind

[Mark W. <ma...@kl...>]

Hi,

On Tue, May 20, 2025 at 07:38:58AM +0200, zz...@ge... wrote:
> This change looks strange to me.
> 
> Am 19.05.25 um 18:53 schrieb Mark Wielaard:
> >https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=859d267a456c2921772f0c957bf24f463c51bd93
> >
> >commit 859d267a456c2921772f0c957bf24f463c51bd93
> >Author: Martin Cermak <mc...@re...>
> >Date:   Mon May 19 11:45:04 2025 +0200
> >
> >     PR504341: Prevent LTP setrlimit05 syscall test from crashing valgrind
> >     Prevent ltp/testcases/kernel/syscalls/setrlimit/setrlimit05 testcase
> >     from crashing valgrind when passing 0xffffffffffff as ARG3 and then
> >     trying to dereference it.
> >     https://bugs.kde.org/show_bug.cgi?id=504341
> [...]
> >index d4653d0273..470635f565 100644
> >--- a/coregrind/m_syswrap/syswrap-linux.c
> >+++ b/coregrind/m_syswrap/syswrap-linux.c
> >@@ -2300,12 +2300,14 @@ PRE(sys_prlimit64)
> >     if (ARG4)
> >        PRE_MEM_WRITE( "rlimit64(old_rlim)", ARG4, sizeof(struct vki_rlimit64) );
> >-   if (ARG3 &&
> >-       ((struct vki_rlimit64 *)(Addr)ARG3)->rlim_cur
> >-        > ((struct vki_rlimit64 *)(Addr)ARG3)->rlim_max) {
> >-      SET_STATUS_Failure( VKI_EINVAL );
> >-   }
> >-   else if (ARG1 == 0 || ARG1 == VG_(getpid)()) {
> >+   if (ARG3) {
> 
> Changing the condition from (ARG3 && rlim_cur > rlim_max) to just
> (ARG3) changes the logic. So some branches below are not executed
> when they should. The switch inside "else if (ARG1 == 0 || ARG1 ==
> VG_(getpid)())" is the main part of prlimit64.

Yeah, my fault. I tried to keep the logic the same by removing the
safe_to_deref from the if clause as Martin originally
suggested. Forgetting that also changes the logic flow :{ And then it
was only tested against the ltp testsuite not the normal
regtests. Apologies.

> The simplest fix I can image is to check the pointers before
> entering the real logic if-else-if block:
> 
> --- a/coregrind/m_syswrap/syswrap-linux.c
> +++ b/coregrind/m_syswrap/syswrap-linux.c
> @@ -2295,19 +2295,28 @@ PRE(sys_prlimit64)
>                   vki_pid_t, pid, unsigned int, resource,
>                   const struct rlimit64 *, new_rlim,
>                   struct rlimit64 *, old_rlim);
> -   if (ARG3)
> +   if (ARG3) {
>        PRE_MEM_READ( "rlimit64(new_rlim)", ARG3, sizeof(struct
> vki_rlimit64) );
> -   if (ARG4)
> -      PRE_MEM_WRITE( "rlimit64(old_rlim)", ARG4, sizeof(struct
> vki_rlimit64) );
> +      if (! ML_(safe_to_deref)((void *)(Addr)ARG3, sizeof(struct
> vki_rlimit64))) {
> +         SET_STATUS_Failure ( VKI_EFAULT );
> +        return;
> +      }
> +   }

yes, doing the checking for safe_to_deref upfront after
PRE_MEM_READ/WRITE and then setting/returning VKI_EFAULT seems the
right idea. Then the logic below can stay the same.

Thanks,

Mark

> -   if (ARG3) {
> -      if (ML_(safe_to_deref)( (void*)(Addr)ARG3, sizeof(struct
> vki_rlimit64) )) {
> -         if (((struct vki_rlimit64 *)(Addr)ARG3)->rlim_cur
> -              > ((struct vki_rlimit64 *)(Addr)ARG3)->rlim_max) {
> -            SET_STATUS_Failure( VKI_EINVAL );
> -         }
> +   if (ARG4) {
> +      PRE_MEM_WRITE( "rlimit64(old_rlim)", ARG4, sizeof(struct
> vki_rlimit64) );
> +      if (! ML_(safe_to_deref)((void *)(Addr)ARG4, sizeof(struct
> vki_rlimit64))) {
> +         SET_STATUS_Failure ( VKI_EFAULT );
> +        return;
>        }
> -   } else if (ARG1 == 0 || ARG1 == VG_(getpid)()) {
> +   }
> +
> +   if (ARG3 &&
> +       ((struct vki_rlimit64 *)(Addr)ARG3)->rlim_cur
> +        > ((struct vki_rlimit64 *)(Addr)ARG3)->rlim_max) {
> +      SET_STATUS_Failure( VKI_EINVAL );
> +   }
> +   else if (ARG1 == 0 || ARG1 == VG_(getpid)()) {
>        switch (ARG2) {
>        case VKI_RLIMIT_NOFILE:
>           SET_STATUS_Success( 0 );
> 
> 
> 
> Regards
> Matthias
>