| Summary: | Missing syswraps for file_getattr and file_setattr | ||
|---|---|---|---|
| Product: | [Developer tools] valgrind | Reporter: | mcermak |
| Component: | general | Assignee: | mcermak |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | mark |
| Priority: | NOR | ||
| Version First Reported In: | 3.26 GIT | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| Latest Commit: | Version Fixed/Implemented In: | ||
| Sentry Crash Report: | |||
| Attachments: |
proposed patch
updated patch |
||
|
Description
mcermak
2025-10-09 06:39:03 UTC
https://lwn.net/Articles/1027795/ fs/file_attr.c commit be7efb2d20d67f334a7de2aef77ae6c69367e646 Author: Andrey Albershteyn <aalbersh@redhat.com> Date: Mon Jun 30 18:20:16 2025 +0200 fs: introduce file_getattr and file_setattr syscalls Introduce file_getattr() and file_setattr() syscalls to manipulate inode extended attributes. The syscalls takes pair of file descriptor and pathname. Then it operates on inode opened accroding to openat() semantics. The struct file_attr is passed to obtain/change extended attributes. This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference that file don't need to be open as we can reference it with a path instead of fd. By having this we can manipulated inode extended attributes not only on regular files but also on special ones. This is not possible with FS_IOC_FSSETXATTR ioctl as with special files we can not call ioctl() directly on the filesystem inode using fd. This patch adds two new syscalls which allows userspace to get/set extended inode attributes on special files by using parent directory and a path - *at() like syscall. Created attachment 185626 [details]
proposed patch
Note that I prepared this patch on top of patch for bug 510169, so there may be related NEWS inaccuracy when applying this. - NEWS entry now needs to move to new 3.27.0 section
- priv_syswrap-linux.h sys_file_[get|set]_attr templates. OK
- vki-scnums-shared-linux.h defines common __NR_file_[get|set]attr. OK
- vki-linux.h defines new vki_file_attr. OK
- syswrap-*-linux.c defines PRE and PRE/POST linux wrappers. OK.
- syswrap-linux.c contains multiple lines > 80 chars, meh.
- The file descriptor checking (arg1) doesn't make sense in sys_file_getattr.
fd == 0 seems a valid file descriptor. And if ARG1 == 0 then of course arg_1 != VKI_AT_FDCWD.
It should at least check whether path is absolute [0] == '/'.
Why exactly can't the logic from fd_at_check_allowed not be used?
- The check should probably be PRE_MEM_WRITE("file_getattr(ufattr)", ARG3, ARG4);
- POST file_getattr is commented out? Should probably also be POST_MEM_WRITE(ARG3, ARG4);
- PRE file_setattr has PRINT for file_GETattr?
- Same odd dfd/filename checking?
- last file_setattr PRE check should probably be PRE_MEM_READ("file_setattr(ufattr)", ARG3, ARG4);
So two issues (apart from formatting and typos). 1) I don't understand why fd_at_check_allowed cannot be used here. If it really cannot then at least the logic must check for the filepath to be safe to read and start with '/'. The size of the struct given as ARG3 is given as ARG4 (not static so kernel can accept larger structs in the future).
Created attachment 187190 [details]
updated patch
Thank you for the review. Updated patch attached, buildbots running...
Tests seem to have passed: https://builder.sourceware.org/buildbot/#/builders/243/builds/113 (In reply to mcermak from comment #6) > Created attachment 187190 [details] > updated patch > > Thank you for the review. Updated patch attached, buildbots running... Looks good. Thanks. ltp tests also pass. Please commit. Commit 3275a177f99d36836107a425b46c84964f1ad354 . |