From: <zz...@ge...> - 2025-05-20 05:39:08
|
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 > > Diff: > --- > NEWS | 1 + > coregrind/m_syswrap/syswrap-linux.c | 14 ++++++++------ > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/NEWS b/NEWS > index d6fbbb41b9..7bb9a79d10 100644 > --- a/NEWS > +++ b/NEWS > @@ -33,6 +33,7 @@ are not entered into bugzilla tend to get forgotten about or ignored. > 501741 syscall cachestat not wrapped > 503969 Make test results of make ltpchecks compatible with bunsen > 504265 FreeBSD: missing syscall wrappers for fchroot and setcred > +504341 Valgrind killed by LTP syscall testcase setrlimit05 > > To see details of a given bug, visit > https://bugs.kde.org/show_bug.cgi?id=XXXXXX > diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c > 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. The testsuite catches this: $ perl tests/vg_regtest none/tests/rlimit{,64}_nofile rlimit_nofile: valgrind ./rlimit_nofile *** rlimit_nofile failed (stderr) *** rlimit64_nofile: valgrind ./rlimit64_nofile *** rlimit64_nofile failed (stderr) *** == 2 tests, 2 stderr failures, 0 stdout failures, 0 stderrB failures, 0 stdoutB failures, 0 post failures == none/tests/rlimit_nofile (stderr) none/tests/rlimit64_nofile (stderr) $ cat none/tests/rlimit64_nofile.stderr.diff --- rlimit64_nofile.stderr.exp 2023-10-27 09:53:13.809659010 +0200 +++ rlimit64_nofile.stderr.out 2025-05-20 07:35:14.513269310 +0200 @@ -1,2 +1,3 @@ +setrlimit64 changing hardlimit must return -1 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; + } + } - 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 |