On Wed, Feb 12, 2025 at 4:58 PM Paulo Alcantara <pc@xxxxxxxxxxxxx> wrote:
>
> Pali Rohár <pali@xxxxxxxxxx> writes:
>
> > On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
> >> Pali Rohár <pali@xxxxxxxxxx> writes:
> >>
> >> > On Wednesday 12 February 2025 17:49:31 Paulo Alcantara wrote:
> >> >> Steve,
> >> >>
> >> >> The commit 438e2116d7bd ("cifs: Change translation of
> >> >> STATUS_PRIVILEGE_NOT_HELD to -EPERM") regressed getcifsacl(1) because it
> >> >> expects -EIO to be returned from getxattr(2) when the client can't read
> >> >> system.cifs_ntsd_full attribute and then fall back to system.cifs_acl
> >> >> attribute. Either -EIO or -EPERM is wrong for getxattr(2), but that's a
> >> >> different problem, though.
> >> >>
> >> >> Reproduced against samba-4.22 server.
> >> >
> >> > That is bad. I can prepare a fix for cifs.ko getxattr syscall to
> >> > translate -EPERM to -EIO. This will ensure that getcifsacl will work as
> >> > before as it would still see -EIO error.
> >>
> >> Sounds good.
> >>
> >> > But as discussed before, we need to distinguish between
> >> > privilege/permission error and other generic errors (access/io).
> >> > So I think that we need 438e2116d7bd commit.
> >>
> >> OK.
> >>
> >> > Based on linux-fsdevel discussion it is a good idea to distinguish
> >> > between errors by mapping status codes to appropriate posix errno, and
> >> > then updating linux syscall manpages.
> >>
> >> Either way, we shouldn't be leaking -EIO or -EPERM to userland from
> >> getxattr(2). By looking at the man pages, -ENODATA seems to be the
> >> appropriate error to return instead.
> >
> > It looks like there are missing error codes for getxattr. Because any
> > path based syscall can return -EACCES if trying to open path to which
> > calling process does not have access.
> >
> > And EACCES is not mentioned nor documented in getxattr(2). Same applies
> > for listxattr(2). Now I have tried listxattr() and it really returns
> > EACCES for /root/file called by nobody.
>
> Both man pages have this:
>
> > In addition, the errors documented in stat(2) can also occur.
>
> and stat(2) actually documents EACCES.
>
> > -EIO is generic I/O error. And I think that this error code could be
> > returned by any I/O syscall when unknown I/O error occurs.
>
> Makes sense.
>
> > Returning -ENODATA for generic or unknown I/O error is a bad idea
> > because ENODATA (= ENOATTR) has already specific meaning when attribute
> > does not exists at all (or process does not have access to it).
>
> You are right.
>
> > For me it makes sense to return -EIO and -EPERM by those syscalls. But
> > for getxattr() we cannot do it due that backward compatibility needed by
> > getcifsacl application.
>
> -EACCES seems the correct one. But yeah, we can't do it due to
> getcifsacl(1) relying on -EIO.
Since EIO is incorrect, we probably should fix getcifsacl ASAP so we
can start returning something more correct for this call e.g. -EACCESS
or -EPERM
Since updating cifs-utils for newer kernels is relatively easy (and
the next version of cifs-utils has some security fixes so will be
easier to rollout), why don't we also change getcifsacl ASAP to handle
the correct rc to give us more freedom for cifs.ko to return the
correct error on newer kernels. Thoughts about this change to
getcifsacl() function which would work with both old and newer kernels
with the rc mapping change? Change to fix the cifs.ko mapping to EIO
could be delayed as well so cifs-utils with the updated check is
rolled out?!
diff --git a/getcifsacl.c b/getcifsacl.c
index 123d11e..3c12789 100644
--- a/getcifsacl.c
+++ b/getcifsacl.c
@@ -447,7 +447,8 @@ getxattr:
free(attrval);
bufsize += BUFSIZE;
goto cifsacl;
- } else if (errno == EIO && !(strcmp(attrname,
ATTRNAME_NTSD_FULL))) {
+ } else if (((errno == EIO) || (errno == EPERM) ||
(errno == EACCES)) &&
+ !(strcmp(attrname, ATTRNAME_NTSD_FULL))) {
/*
* attempt to fetch SACL in addition to owner
and DACL via
* ATTRNAME_NTSD_FULL, fall back to owner/DACL via
Thanks,
Steve
diff --git a/getcifsacl.c b/getcifsacl.c
index 123d11e..a566dd7 100644
--- a/getcifsacl.c
+++ b/getcifsacl.c
@@ -447,7 +447,8 @@ getxattr:
free(attrval);
bufsize += BUFSIZE;
goto cifsacl;
- } else if (errno == EIO && !(strcmp(attrname, ATTRNAME_NTSD_FULL))) {
+ } else if (((errno == EIO) || (errno == EPERM) || (errno == EACCES)) &&
+ !(strcmp(attrname, ATTRNAME_NTSD_FULL))) {
/*
* attempt to fetch SACL in addition to owner and DACL via
* ATTRNAME_NTSD_FULL, fall back to owner/DACL via
