| Summary: | Memory allegedly uninitialized after ioctl(PROCMAP_QUERY) | ||
|---|---|---|---|
| Product: | [Developer tools] valgrind | Reporter: | blubban |
| Component: | memcheck | Assignee: | mcermak |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | mark |
| Priority: | NOR | ||
| Version First Reported In: | 3.25.0 | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| Latest Commit: | Version Fixed/Implemented In: | ||
| Sentry Crash Report: | |||
| Attachments: |
WIP hotfix that avoids the false positive.
proposed patch additional patch addressing Comment#3 updated patch |
||
Created attachment 186956 [details]
WIP hotfix that avoids the false positive.
Created attachment 186990 [details] proposed patch Also checked in to try-branch so that it can get buildbot-tested: https://sourceware.org/git/?p=valgrind.git;a=shortlog;h=refs/heads/users/mcermak/try-bug508328 I can't determine how (or if) the first patch works, but the second one looks good to my (admittedly inexperienced in this project) eyes.
Except the kernel will use the build_id_{size,addr} fields the same way as vma_name_{size,addr}. You should duplicate that.
Created attachment 187024 [details] additional patch addressing Comment#3 Agreed. Follow-up patch addressing Comment#3 attached. Try-branch updated. The previous patch passed testing [1]. Buildbots should pick up this update soon.. [1] https://builder.sourceware.org/buildbot/#/builders/243/builds/109 The new testcase needs to be moved under memcheck/tests/linux since it is linux kernel specific. You will want to check ML_(safe_to_deref) (pq, sizeof(struct vki_procmap_query) in the PRE handler before doing any checks that need dereferencing pq. Also check the other "in" fields (size, query_flags and query_addr) with PRE_FIELD_READ in the PRE handler. Also mark the other "out" fields (vma_*, inode, dev_*) as written with POST_FIELD_WRITE in the POST handler. (In the POST handler you can probably just assume everything is written/defined with POST_MEM_WRITE(pq, pq-<size) instead of doing individual POST_FIELD_WRITEs) Also looking at the try builds (thanks for doing those!) it looks like for the testcase you also need a configure check to make sure PROCMAP_QUERY is defined in linux/fs.h. https://builder.sourceware.org/buildbot/#/changes/102828 shows various systems with an older kernel that doesn't define it. Thank you for your review! I've addressed your comments in my branch users/mcermak/try-bug508328: $ git log --pretty --oneline -4 6605bba9b (HEAD -> users/mcermak/try-bug508328, origin/users/mcermak/try-bug508328) PR508328: Run ioctl_procmap_query.vgtest conditionally 0a779b583 PR508328: Reflect the testcase move in .gitignore b99a16afb PR508328: Check all the members of struct procmap_query 257ba83ca PR508328: Move ioctl_procmap_query.vgtest to the linux subdirectory $ https://sourceware.org/git/?p=valgrind.git;a=shortlog;h=refs/heads/users/mcermak/try-bug508328 Buildbot test results green: https://builder.sourceware.org/buildbot/#/builders/243/builds/111 . Could you post that as a squash patch? I think it looks good. I would only remove the SET_STATUS_Failure(VKI_EFAULT) after the safe_to_deref check. The kernel will return that anyway, but it might accept a larger/smaller struct in the future? Created attachment 187159 [details]
updated patch
Attaching squashed patch addressing the previous comment.
(In reply to mcermak from comment #10) > Created attachment 187159 [details] > updated patch > > Attaching squashed patch addressing the previous comment. Looks good. Since it might be that the testcase isn't build (if the configure check fails) I think you should have something like: prereq: test -x ioctl_procmap_query in ioctl_procmap_query.vgtest otherwise you get failures on old kernels, where the test should be skipped. Please push with that change. Makes sense, thanks a ton for the review. Pushed as commit 2b4b31de6965c3af5ecaca2c33b039eb72ec3706 . |
#include <stdio.h> #include <stdint.h> #include <fcntl.h> #include <sys/ioctl.h> #include <linux/fs.h> int main(int argc, char** argv) { char name[256]; struct procmap_query pq = { .size = sizeof(pq), .query_addr = (uintptr_t)main, .vma_name_size = 256, .vma_name_addr = (uintptr_t)name }; int fd = open("/proc/self/maps", O_RDONLY); ioctl(fd, PROCMAP_QUERY, &pq); puts(name); } Install Linux >= 6.11, then compile and run the above. No particular flags needed, gcc's and valgrind's defaults are fine. Expected: Print the usual Valgrind header/footer, and the path of the executable. Actual: Also prints various warnings about the name variable being uninitialized. Discovered on Debian stable (3.24), also reproduces on 3.25.1. Probably needs something similar to bug 333817.