| Summary: | readlink("/proc/self/exe") overwrites buffer beyond its return value | ||
|---|---|---|---|
| Product: | [Developer tools] valgrind | Reporter: | akuviljanen17 |
| Component: | general | Assignee: | Paul Floyd <pjfloyd> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | mark, pjfloyd |
| Priority: | NOR | ||
| Version First Reported In: | 3.26.0 | ||
| Target Milestone: | --- | ||
| Platform: | Debian stable | ||
| OS: | Linux | ||
| Latest Commit: | Version Fixed/Implemented In: | ||
| Sentry Crash Report: | |||
| Attachments: |
initial patch
Updated patch |
||
I bisected this, the bug first appeared in commit 0690dc39644d15fc89813419ffcdf9754b098260. The same problem exists with readlnkat using an absolute path (and probably any combination of dirfd being /, /proc, /proc/self, /proc/pid and the path being the relative remainder to /proc/pid/exe or /proc/self/exe, but that's a corner case that we don't handle at all).
The man page says
readlink() places the contents of the symbolic link pathname in the buffer buf,
which has size bufsiz. readlink() does not append a terminating null byte to buf.
It will (silently) truncate the contents (to a length of bufsiz characters), in case
the buffer is too small to hold all of the contents.
Since we aren't null terminating we may be within the letter of the law. I don't think that's a good enough excuse.
Here is what is happening
1. PRE does its stuff
2. Valgrind does the client syscall which resolves the link to the tool exe
3. The post sees that it is a special case and does another syscall which resolves to the guest exename
4. The buffer gets overwritten without null termination.
What we really need to do is to move the special case syscall to the PRE function.
Created attachment 188184 [details]
initial patch
Also needs doing for Linux realpathat (with testcase)
(In reply to Paul Floyd from comment #2) > The same problem exists with readlnkat using an absolute path (and probably > any combination of dirfd being /, /proc, /proc/self, /proc/pid and the path > being the relative remainder to /proc/pid/exe or /proc/self/exe, but that's > a corner case that we don't handle at all). > > The man page says > readlink() places the contents of the symbolic link pathname in > the buffer buf, > which has size bufsiz. readlink() does not append a terminating null > byte to buf. > It will (silently) truncate the contents (to a length of bufsiz > characters), in case > the buffer is too small to hold all of the contents. > > Since we aren't null terminating we may be within the letter of the law. I > don't think that's a good enough excuse. > > Here is what is happening > > 1. PRE does its stuff > 2. Valgrind does the client syscall which resolves the link to the tool exe > 3. The post sees that it is a special case and does another syscall which > resolves to the guest exename > 4. The buffer gets overwritten without null termination. > > What we really need to do is to move the special case syscall to the PRE > function. Are you sure? commit 0690dc39644d15fc89813419ffcdf9754b098260 says the special syscall was done/moved explicitly into the POST handler: commit 0690dc39644d15fc89813419ffcdf9754b098260 Author: Mark Wielaard <mark@klomp.org> Date: Sun Sep 22 23:24:34 2024 +0200 Implement /proc/self/exe readlink[at] fallback in POST handler Calling the readlink[at] syscall directly from the PRE handler defeats the FUSE_COMPATIBLE_MAY_BLOCK (SfMayBlock) flag. Add a POST handler that only explicitly calls the readlink[at] handler for the /proc/self/exe fallback (this should be fine unless /proc is also implemented as fuse in this process). Adjust readlink[at] GENX_ and LINX_ syswrap macros to GENXY and LINXY. https://bugs.kde.org/show_bug.cgi?id=493507 So moving it back into the PRE handler will reintroduce that bug. I didn't check the commit message of 0690dc39644d15fc89813419ffcdf9754b098260
How about instead of
SET_STATUS_from_SysRes( VG_(do_syscall3)(saved, (UWord)name,
ARG2, ARG3));
something like
Int oldlen = RET;
SysRes newret = do_syscall3)(saved, (UWord)name, ARG2, ARG3);
Int newlen = sr_Res(newret);
if (newlen < oldlen)
VG_(memset)((void*)(ARG2+newlen, 0, oldlen-newlen);
(not yet tested)
Created attachment 188193 [details]
Updated patch
This time simply don't make a syscall if the path is a /proc/self/exe like thing. Just use VG_(resolved_executable).
commit 5c0f5e604bc3a4e8822ea59c488c123af6284afd (HEAD -> master, origin/master, origin/HEAD, bug514094) Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sat Jan 3 13:48:50 2026 +0100 Bug 514094 - readlink("/proc/self/exe") overwrites buffer beyond its return value Used the reproducer as the basis for a test on Solaris and Linux. The attached patch doesn't seem quite right; it overflows the buffer if the path doesn't fit.
In my original C code, change `readlink("/proc/self/exe", buf, 64)` to `readlink("/proc/self/exe", buf, 5)`. I would expect to get `ret = 5, buf = /home`, but with the patch I get `ret = 17, buf = /home/akuli/a.out`.
I think we want something like `SizeT res = min(VG_(strlen)(VG_(resolved_exename)), ARG3)` followed by a memcpy with size `res`.
You are right. I did think of that bit missed it out when juggling between Linux and Solaris testing. Will update in a moment along with readlinkat for Linux. commit bd9edb8fcd0a8692d865e08fab2a573a4cde4c16 (HEAD -> master, origin/master, origin/HEAD) Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sat Jan 3 21:17:00 2026 +0100 readlink[at] syswrap: limit copy to bufsiz when path is proc self exe I don't think this is right, you are still doing a (In reply to Paul Floyd from comment #10) > commit bd9edb8fcd0a8692d865e08fab2a573a4cde4c16 (HEAD -> master, > origin/master, origin/HEAD) > Author: Paul Floyd <pjfloyd@wanadoo.fr> > Date: Sat Jan 3 21:17:00 2026 +0100 > > readlink[at] syswrap: limit copy to bufsiz when path is proc self exe This seems to add a testcase readlinkat_self.c which wasn't checked in. Using the resolved_exename directly instead of going through do_syscall (readlink) with cl_exec_fd is smart.
So we don't have a do_syscall invocation in the PRE handler. Nice.
But this looks dangerous:
+ HChar* out_name = (HChar*)ARG3;
+ SizeT res = VG_(strlen)(VG_(resolved_exename));
+ res = VG_MIN(res, ARG4);
+ VG_(strncpy)(out_name, VG_(resolved_exename), res);
We never check ARG3 (ARG2 for readlink) is fully writable, so may crash on the strncpy.
I think we need a check like:
diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c
index 406e6960b0bb..aa4a0caf11f9 100644
--- a/coregrind/m_syswrap/syswrap-generic.c
+++ b/coregrind/m_syswrap/syswrap-generic.c
@@ -5096,8 +5096,12 @@ PRE(sys_readlink)
HChar* out_name = (HChar*)ARG2;
SizeT res = VG_(strlen)(VG_(resolved_exename));
res = VG_MIN(res, ARG3);
- VG_(strncpy)(out_name, VG_(resolved_exename), res);
- SET_STATUS_Success(res);
+ if (ML_(safe_to_deref)(out_name, res)) {
+ VG_(strncpy)(out_name, VG_(resolved_exename), res);
+ SET_STATUS_Success(res);
+ } else {
+ SET_STATUS_Failure(VKI_EFAULT);
+ }
fuse_may_block = False;
}
}
I've added the missing files. I'll look at checking buf tomorrow. I think that, finally, this is OK now. (In reply to Paul Floyd from comment #14) > I think that, finally, this is OK now. Yes, looks good. For the record this consisted of the following commits: commit bf154d815a9fd7f4aaae97e31aa03cecf69448d4 Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sat Jan 3 18:24:34 2026 +0100 Solaris: set VG_(resolved_exename) in load_client() Haven't needed it yet, but I would like to try using it in the readlink syscall wrapper. commit 5c0f5e604bc3a4e8822ea59c488c123af6284afd Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sat Jan 3 13:48:50 2026 +0100 Bug 514094 - readlink("/proc/self/exe") overwrites buffer beyond its return value Used the reproducer as the basis for a test on Solaris and Linux. commit bd9edb8fcd0a8692d865e08fab2a573a4cde4c16 Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sat Jan 3 21:17:00 2026 +0100 readlink[at] syswrap: limit copy to bufsiz when path is proc self exe commit 987034c44105cdc2f6f8d84751135d23bd5c37b6 Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sat Jan 3 22:16:58 2026 +0100 Regtest: add missing readlinkat_self files commit dae37ecd2692e0e5beba77c296c2648ebbf47637 Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sat Jan 3 22:32:56 2026 +0100 regtest: fix warning Added a nice const named variable then didn't use it commit 294742a2d9f431fd2dcd73db161f67fb12ddd833 Author: Mark Wielaard <mark@klomp.org> Date: Sat Jan 3 22:50:28 2026 +0100 Fix bug514094,vgtest typo in none/tests/Makefile.am commit 8d8023d107699c7c2d97acf2dcb77bae71c0b1cf Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sun Jan 4 09:34:54 2026 +0100 syswrap readlink and linux readlinkat: check that buf is accessible for proc self exe case Also update the t testcases to cover this. commit 827a1b8c307a2eafa001788565e14af3445f2151 Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sun Jan 4 10:17:15 2026 +0100 solaris readlinkat: check that buf is accessible for proc self path a.out I am going to try to backport this to the (not yet existing) VALGRIND_3_26_BRANCH because it can be considered a regression. (In reply to Mark Wielaard from comment #15) > I am going to try to backport this to the (not yet existing) > VALGRIND_3_26_BRANCH because it can be considered a regression. Pushed as one big squashed cherry-pick to the branch: commit 7de247c998049db64c4df8cb8bc8e481493f3b8e Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sat Jan 3 18:24:34 2026 +0100 readlink("/proc/self/exe") overwrites buffer beyond its return value https://bugs.kde.org/show_bug.cgi?id=514094 Squashed cherry-picks: Solaris: set VG_(resolved_exename) in load_client() Haven't needed it yet, but I would like to try using it in the readlink syscall wrapper. (cherry picked from commit bf154d815a9fd7f4aaae97e31aa03cecf69448d4) Bug 514094 - readlink("/proc/self/exe") overwrites buffer beyond its return value Used the reproducer as the basis for a test on Solaris and Linux. (cherry picked from commit 5c0f5e604bc3a4e8822ea59c488c123af6284afd) readlink[at] syswrap: limit copy to bufsiz when path is proc self exe (cherry picked from commit bd9edb8fcd0a8692d865e08fab2a573a4cde4c16) Regtest: add missing readlinkat_self files (cherry picked from commit 987034c44105cdc2f6f8d84751135d23bd5c37b6) regtest: fix warning Added a nice const named variable then didn't use it (cherry picked from commit dae37ecd2692e0e5beba77c296c2648ebbf47637) Fix bug514094,vgtest typo in none/tests/Makefile.am (cherry picked from commit 294742a2d9f431fd2dcd73db161f67fb12ddd833) syswrap readlink and linux readlinkat: check that buf is accessible for proc self exe case Also update the t testcases to cover this. (cherry picked from commit 8d8023d107699c7c2d97acf2dcb77bae71c0b1cf) solaris readlinkat: check that buf is accessible for proc self path a.out (cherry picked from commit 827a1b8c307a2eafa001788565e14af3445f2151) |
SUMMARY On Debian 13, the following C program behaves differently under valgrind vs not under valgrind: #include <stdio.h> #include <unistd.h> #include <string.h> #include <stdio.h> int main() { char buf[64]; memset(buf, 0, 64); int ret = readlink("/proc/self/exe", buf, 64); printf("ret = %d, buf = %.64s\n", ret, buf); return 0; } STEPS TO REPRODUCE 1. Compile the program: `cc foo.c` 2. Run without valgrind: `./a.out` 3. Run with valgrind: `valgrind ./a.out` OBSERVED RESULT Without valgrind: `ret = 17, buf = /home/akuli/a.out` With valgrind: `ret = 17, buf = /home/akuli/a.outrind/memcheck-amd64-linux` Return value 17 is correct, but the rest of the buffer is overwritten by the string `/usr/libexec/valgrind/memcheck-amd64-linux`. EXPECTED RESULT Always `ret = 17, buf = /home/akuli/a.out` SOFTWARE/OS VERSIONS Debian 13 Valgrind 3.24.0 Valgrind 3.26.0 ADDITIONAL INFORMATION This is not specific to memcheck: adding `--tool=none` gives me `ret = 17, buf = /home/akuli/a.outrind/none-amd64-linux`. I tried with valgrind 3.24.0 from apt and 3.26.0 compiled from source (valgrind.org), same behavior. I think this used to work in some version of valgrind, because I got this problem when valgrinding after I upgraded to a newer Debian. This only happens with `/proc/self/exe`. If I use another symlink, it works fine. `strace ./a.out` gives `readlink("/proc/self/exe", "/home/akuli/a.out", 64)`. This can be worked around with `buf[ret] = '\0'` or by placing the program to a directory with a long name. This is not a compiler problem, because this happens with gcc and with LLVM.