On 2/6/25 11:03, Christian Brauner brauner@kernel.org wrote:
On Thu, Feb 06, 2025 at 09:51:33AM +0000, Mike Yuan wrote:
On 2/6/25 10:31, Christian Brauner brauner@kernel.org wrote:
On Wed, Feb 05, 2025 at 08:47:23PM +0000, Mike Yuan wrote:
Cited from commit message of original patch [1]:
One use case will be setfiles(8) setting SELinux file contexts ("security.selinux") without race conditions and without a file descriptor opened with read access requiring SELinux read permission.
Also, generally all *at() syscalls operate on O_PATH fds, unlike f*() ones. Yet the O_PATH fds are rejected by *xattrat() syscalls in the final version merged into tree. Instead, let's switch things to CLASS(fd_raw).
Note that there's one side effect: f*xattr() starts to work with O_PATH fds too. It's not clear to me whether this is desirable (e.g. fstat() accepts O_PATH fds as an outlier).
[1] https://lore.kernel.org/all/20240426162042.191916-1-cgoettsche@seltendoof.de...
Fixes: 6140be90ec70 ("fs/xattr: add *at family syscalls") Signed-off-by: Mike Yuan me@yhndnzj.com Cc: Al Viro viro@zeniv.linux.org.uk Cc: Christian Göttsche cgzones@googlemail.com Cc: Christian Brauner brauner@kernel.org Cc: stable@vger.kernel.org
I expanded on this before. O_PATH is intentionally limited in scope and it should not allow to perform operations that are similar to a read or write which getting and setting xattrs is.
Patches that further weaken or dilute the semantics of O_PATH are not acceptable.
But the *at() variants really should be able to work with O_PATH fds, otherwise they're basically useless? I guess I just need to keep f*() as-is?
I'm confused. If you have: filename = getname_maybe_null(pathname, at_flags); if (!filename) { CLASS(fd, f)(dfd); if (fd_empty(f)) error = -EBADF; else error = file_setxattr(fd_file(f), &ctx); Then this branch ^^ cannot use fd_raw because you're allowing operations directly on the O_PATH file descriptor. Using the O_PATH file descriptor for lookup is obviously fine which is why the other branch exists: } else { error = filename_setxattr(dfd, filename, lookup_flags, &ctx); } IOW, your patch makes AT_EMPTY_PATH work with an O_PATH file descriptor which isn't acceptable. However, it is already perfectly fine to use an O_PATH file descriptor for lookup.
Well, again, [1] clearly stated the use case:
Those can be used to operate on extended attributes,
especially security related ones, either relative to a pinned directory or [on a file descriptor without read access, avoiding a /proc/<pid>/fd/<fd> detour, requiring a mounted procfs].
And this surfaced in my PR to systemd:
https://github.com/systemd/systemd/pull/36228/commits/34fe16fb177d2f917570c5...
How are *xattrat() syscalls different from e.g. fchmodat2(AT_EMPTY_PATH) in that regard? I can agree that the semantics of f*xattr() ought to be left untouched, yet I fail to grok the case for _at variants.
fs/xattr.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/xattr.c b/fs/xattr.c index 02bee149ad96..15df71e56187 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -704,7 +704,7 @@ static int path_setxattrat(int dfd, const char __user *pathname,
filename = getname_maybe_null(pathname, at_flags); if (!filename) {
CLASS(fd, f)(dfd);
CLASS(fd_raw, f)(dfd);@@ -848,7 +848,7 @@ static ssize_t path_getxattrat(int dfd, const char __user *pathname,
filename = getname_maybe_null(pathname, at_flags); if (!filename) {
CLASS(fd, f)(dfd);
CLASS(fd_raw, f)(dfd);@@ -978,7 +978,7 @@ static ssize_t path_listxattrat(int dfd, const char __user *pathname,
filename = getname_maybe_null(pathname, at_flags); if (!filename) {
CLASS(fd, f)(dfd);
CLASS(fd_raw, f)(dfd);@@ -1079,7 +1079,7 @@ static int path_removexattrat(int dfd, const char __user *pathname,
filename = getname_maybe_null(pathname, at_flags); if (!filename) {
CLASS(fd, f)(dfd);
CLASS(fd_raw, f)(dfd);base-commit: a86bf2283d2c9769205407e2b54777c03d012939
2.48.1