Regression with getcifsacl(1) in v6.14-rc1

Regression with getcifsacl(1) in v6.14-rc1

[Paulo Alcantara]

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.

Re: Regression with getcifsacl(1) in v6.14-rc1

[Pali Rohár]

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.

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.

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.

Re: Regression with getcifsacl(1) in v6.14-rc1

[Paulo Alcantara]

Pali Rohár <pali@kernel.org> 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.

Re: Regression with getcifsacl(1) in v6.14-rc1

[Pali Rohár]

On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
> Pali Rohár <pali@kernel.org> 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.

-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.

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).

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.

Re: Regression with getcifsacl(1) in v6.14-rc1

[Paulo Alcantara]

Pali Rohár <pali@kernel.org> writes:

> On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
>> Pali Rohár <pali@kernel.org> 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.

Re: Regression with getcifsacl(1) in v6.14-rc1

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 4361 bytes --]

On Wed, Feb 12, 2025 at 4:58 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Pali Rohár <pali@kernel.org> writes:
>
> > On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
> >> Pali Rohár <pali@kernel.org> 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

[-- Attachment #2: getcifsacl.diff --]
[-- Type: text/x-patch, Size: 523 bytes --]

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

Re: Regression with getcifsacl(1) in v6.14-rc1

[Pali Rohár]

On Wednesday 12 February 2025 17:47:19 Steve French wrote:
> On Wed, Feb 12, 2025 at 4:58 PM Paulo Alcantara <pc@manguebit.com> wrote:
> >
> > Pali Rohár <pali@kernel.org> writes:
> >
> > > On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
> > >> Pali Rohár <pali@kernel.org> 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?!

That should work too.

Anyway, if I'm looking correctly at that getcifsacl.c code, it contains
fallback from fetching SACL+DACL attribute (ATTRNAME_NTSD_FULL) to
DACL-only attribute.

And if the user does not have permission to access SACL then
STATUS_PRIVILEGE_NOT_HELD is returned by the SMB server.
STATUS_PRIVILEGE_NOT_HELD is being mapped to EPERM.

So EACCES should not be needed there.

If SMB server returns STATUS_ACCESS_DENIED (EACCES) then it means that
user does not have access to path or DACL, and so fallback from
SACL+DACL (ATTRNAME_NTSD_FULL) to DACL-only attribute is useless.

> 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

Re: Regression with getcifsacl(1) in v6.14-rc1

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 6129 bytes --]

ok - how about this updated patch to getcifsacl?

Presumably the rc check for the getcifsacl retry is only needed by
more cifs-utils in last four years
due to this patch.

Author: Boris Protopopov <pboris@amazon.com>
Date:   Thu Nov 19 21:40:42 2020 +0000

    Extend cifs acl utilities to handle SACLs

    Extend getcifsacl/setcifsacl utilities to handle System ACLs (SACLs)
    in addition to Discretionary ACLs (DACLs). The SACL extensions depend
    on CIFS client support for system.cifs_ntsd_full extended attribute.

On Wed, Feb 12, 2025 at 6:02 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 12 February 2025 17:47:19 Steve French wrote:
> > On Wed, Feb 12, 2025 at 4:58 PM Paulo Alcantara <pc@manguebit.com> wrote:
> > >
> > > Pali Rohár <pali@kernel.org> writes:
> > >
> > > > On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
> > > >> Pali Rohár <pali@kernel.org> 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?!
>
> That should work too.
>
> Anyway, if I'm looking correctly at that getcifsacl.c code, it contains
> fallback from fetching SACL+DACL attribute (ATTRNAME_NTSD_FULL) to
> DACL-only attribute.
>
> And if the user does not have permission to access SACL then
> STATUS_PRIVILEGE_NOT_HELD is returned by the SMB server.
> STATUS_PRIVILEGE_NOT_HELD is being mapped to EPERM.
>
> So EACCES should not be needed there.
>
> If SMB server returns STATUS_ACCESS_DENIED (EACCES) then it means that
> user does not have access to path or DACL, and so fallback from
> SACL+DACL (ATTRNAME_NTSD_FULL) to DACL-only attribute is useless.
>
> > 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



-- 
Thanks,

Steve

[-- Attachment #2: 0001-getcifsacl-fix-return-code-check-for-getting-full-AC.patch --]
[-- Type: text/x-patch, Size: 1503 bytes --]

From 6b1bd1d5f96fd64fc697ccde671d63799c75ed8a Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Wed, 12 Feb 2025 20:22:00 -0600
Subject: [PATCH] getcifsacl: fix return code check for getting full ACL

Older clients incorrectly mapped STATUS_PRIVILEGE_NOT_HELD to
EIO instead of EPERM, so make sure we check for either of these rc
when deciding whether to fall back to querying just the DACL (and
owner) when  querying the complete ACL including SACL (ie Auditing
information) fails with insufficient permissions.

Cc: Boris Protopopov <pboris@amazon.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 getcifsacl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/getcifsacl.c b/getcifsacl.c
index 123d11e..97471e9 100644
--- a/getcifsacl.c
+++ b/getcifsacl.c
@@ -447,12 +447,13 @@ getxattr:
 			free(attrval);
 			bufsize += BUFSIZE;
 			goto cifsacl;
-		} else if (errno == EIO && !(strcmp(attrname, ATTRNAME_NTSD_FULL))) {
+		} else if (((errno == EIO) || (errno == EPERM)) &&
+			   !(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
 			 * ATTRNAME_ACL if not allowed
-			 * CIFS client maps STATUS_PRIVILEGE_NOT_HELD to EIO
+			 * Older CIFS client maps STATUS_PRIVILEGE_NOT_HELD to EIO instead of EPERM
 			 */
 			fprintf(stderr, "WARNING: Insufficient priviledges to fetch SACL for %s\n",
 				filename);
-- 
2.43.0

Re: Regression with getcifsacl(1) in v6.14-rc1

[Paulo Alcantara]

Steve French <smfrench@gmail.com> writes:

> ok - how about this updated patch to getcifsacl?

LGTM.  Please also make sure to add:

Reported-by: Jianhong Yin <jiyin@redhat.com>
Closes: https://lore.kernel.org/r/2bdf635d3ebd000480226ee8568c32fb@manguebit.com
Reviewed-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>

Re: Regression with getcifsacl(1) in v6.14-rc1

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
> Pali Rohár <pali@kernel.org> 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.

Now I quickly prepared a fix, it is straightforward but I have not
tested it yet. Testing requires non-admin user which does not have
SeSecurityPrivilege privilege configured. Could you check if it is
fixing this problem?

[-- Attachment #2: 0001-cifs-Reports-EIO-for-getxattr-system.cifs_ntsd_full-.patch --]
[-- Type: text/x-diff, Size: 2534 bytes --]

From c9032ef054acd0a92ea641dc33a7b62a5833e00c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pali=20Roh=C3=A1r?= <pali@kernel.org>
Date: Thu, 13 Feb 2025 19:37:29 +0100
Subject: [PATCH] cifs: Reports -EIO for getxattr(system.cifs_ntsd_full)
 privilege error
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit 438e2116d7bd ("cifs: Change translation of STATUS_PRIVILEGE_NOT_HELD
to -EPERM") globally changed translation of STATUS_PRIVILEGE_NOT_HELD
status code from -EIO to -EPERM which is more appropriate errno code.

Unfortunately it broke getcifsacl utility when called by user which does
not have SeSecurityPrivilege privilege, which is required to fetch SACLs.

Userspace utility getcifsacl expects that kernel reports privilege error
for system.cifs_ntsd_full xattr as EIO errno, not as EPERM errno.

When privilege error via EIO errno is detected then getcifsacl request
security descriptor without SACLs via system.cifs_acl xattr. This is
allowed also without SeSecurityPrivilege privilege.

This change fixes the errno returned by getxattr(system.cifs_ntsd_full)
call, as required by backward compatibility for getcifsacl utility.
With this change EIO is returned as before the mentioned commit.

Fixes: 438e2116d7bd ("cifs: Change translation of STATUS_PRIVILEGE_NOT_HELD to -EPERM")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/smb/client/xattr.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/smb/client/xattr.c b/fs/smb/client/xattr.c
index 58a584f0b27e..11207979c630 100644
--- a/fs/smb/client/xattr.c
+++ b/fs/smb/client/xattr.c
@@ -331,6 +331,20 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
 			rc = PTR_ERR(pacl);
 			cifs_dbg(VFS, "%s: error %zd getting sec desc\n",
 				 __func__, rc);
+			if (rc == -EPERM && handler->flags == XATTR_CIFS_NTSD_FULL) {
+				/*
+				 * Report STATUS_PRIVILEGE_NOT_HELD error (signaled by -EPERM)
+				 * to userspace as EIO errno for system.cifs_ntsd_full xattr.
+				 * This is backward compatibility for old version of getcifsacl
+				 * utility which is doing fallback from system.cifs_ntsd_full xattr
+				 * to system.cifs_acl xattr when user does not have privilege to
+				 * fetch SACL and expects that kernel reports insufficient privilege
+				 * error via EIO errno (instead of EPERM errno).
+				 */
+				rc = -EIO;
+				cifs_dbg(FYI, "%s: error changed to %zd for compatibility\n",
+					 __func__, rc);
+			}
 		} else {
 			if (value) {
 				if (acllen > size)
-- 
2.20.1

Re: Regression with getcifsacl(1) in v6.14-rc1

[Pali Rohár]

On Wednesday 12 February 2025 20:46:35 Steve French wrote:
> ok - how about this updated patch to getcifsacl?

Looks good.

As an additional improvement it would be nice to show exact reason what
is missing in error message - missing SeSecurityPrivilege privilege.
Because just generic error "Insufficient priviledges" does not say what
exactly is missing.

Re: Regression with getcifsacl(1) in v6.14-rc1

[Steve French]

This change to fs/smb/client/xattr.c is probably safe, and presumably
could be removed in future kernels in a year or two as the updated
cifs-utils which properly checks the error codes is rolled out
broadly.

On Thu, Feb 13, 2025 at 12:42 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
> > Pali Rohár <pali@kernel.org> 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.
>
> Now I quickly prepared a fix, it is straightforward but I have not
> tested it yet. Testing requires non-admin user which does not have
> SeSecurityPrivilege privilege configured. Could you check if it is
> fixing this problem?



-- 
Thanks,

Steve

Re: Regression with getcifsacl(1) in v6.14-rc1

[Pali Rohár]

Hello, I would like to ask, how you handled this regression? Have you
taken this my fix to address it? Or is it going to be addresses in other
way?

On Thursday 13 February 2025 12:52:50 Steve French wrote:
> This change to fs/smb/client/xattr.c is probably safe, and presumably
> could be removed in future kernels in a year or two as the updated
> cifs-utils which properly checks the error codes is rolled out
> broadly.
> 
> On Thu, Feb 13, 2025 at 12:42 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
> > > Pali Rohár <pali@kernel.org> 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.
> >
> > Now I quickly prepared a fix, it is straightforward but I have not
> > tested it yet. Testing requires non-admin user which does not have
> > SeSecurityPrivilege privilege configured. Could you check if it is
> > fixing this problem?
> 
> 
> 
> -- 
> Thanks,
> 
> Steve

Re: Regression with getcifsacl(1) in v6.14-rc1

[Paulo Alcantara]

Pali Rohár <pali@kernel.org> writes:

> Hello, I would like to ask, how you handled this regression? Have you
> taken this my fix to address it? Or is it going to be addresses in other
> way?

It's already fixed in cifs-utils-7.2 by commit 8b4b6e459d2a
("getcifsacl: fix return code check for getting full ACL").

Re: Regression with getcifsacl(1) in v6.14-rc1

[Pali Rohár]

On Sunday 23 March 2025 21:36:56 Paulo Alcantara wrote:
> Pali Rohár <pali@kernel.org> writes:
> 
> > Hello, I would like to ask, how you handled this regression? Have you
> > taken this my fix to address it? Or is it going to be addresses in other
> > way?
> 
> It's already fixed in cifs-utils-7.2 by commit 8b4b6e459d2a
> ("getcifsacl: fix return code check for getting full ACL").

Ok, and into kernel is not going to be addressed that regression for
older cifs-utils?

Re: Regression with getcifsacl(1) in v6.14-rc1

[Steve French]

On Mon, Mar 24, 2025 at 3:23 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Sunday 23 March 2025 21:36:56 Paulo Alcantara wrote:
> > Pali Rohár <pali@kernel.org> writes:
> >
> > > Hello, I would like to ask, how you handled this regression? Have you
> > > taken this my fix to address it? Or is it going to be addresses in other
> > > way?
> >
> > It's already fixed in cifs-utils-7.2 by commit 8b4b6e459d2a
> > ("getcifsacl: fix return code check for getting full ACL").
>
> Ok, and into kernel is not going to be addressed that regression for
> older cifs-utils?

I thought we had decided that it was risky to intentionally return the
wrong return code to
userspace, to workaround an app bug (especially since it is easier to update
cifs-utils than update the kernel, and also since cifs-utils update to
7.3 is strongly
encouraged for users due to multiple security issues fixed)

-- 
Thanks,

Steve

Re: Regression with getcifsacl(1) in v6.14-rc1

[Pali Rohár]

On Monday 24 March 2025 10:33:06 Steve French wrote:
> On Mon, Mar 24, 2025 at 3:23 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Sunday 23 March 2025 21:36:56 Paulo Alcantara wrote:
> > > Pali Rohár <pali@kernel.org> writes:
> > >
> > > > Hello, I would like to ask, how you handled this regression? Have you
> > > > taken this my fix to address it? Or is it going to be addresses in other
> > > > way?
> > >
> > > It's already fixed in cifs-utils-7.2 by commit 8b4b6e459d2a
> > > ("getcifsacl: fix return code check for getting full ACL").
> >
> > Ok, and into kernel is not going to be addressed that regression for
> > older cifs-utils?
> 
> I thought we had decided that it was risky to intentionally return the
> wrong return code to
> userspace, to workaround an app bug (especially since it is easier to update
> cifs-utils than update the kernel, and also since cifs-utils update to
> 7.3 is strongly
> encouraged for users due to multiple security issues fixed)
> 
> -- 
> Thanks,
> 
> Steve

Ok, fine. I just wanted to be sure that we have not forgot for something.

Re: Regression with getcifsacl(1) in v6.14-rc1

[Steve French]

> I just wanted to be sure that we have not forgot for something

On that subject, given the business of the week (LSF/MM/Storage
summit) and merge window opening up, it is very possible we could miss
patches (and over 40 of yours to still rereview as well, and decide
what is safest to cherrypick of those) - so let me know if I have
missed critical patches.   There are some very urgent ones (e.g.
finding out why multichannel to ksmbd broke just before 6.13 kernel
came out) and the directory lease fixes (and perf improvements)

On Mon, Mar 24, 2025 at 12:07 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Monday 24 March 2025 10:33:06 Steve French wrote:
> > On Mon, Mar 24, 2025 at 3:23 AM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Sunday 23 March 2025 21:36:56 Paulo Alcantara wrote:
> > > > Pali Rohár <pali@kernel.org> writes:
> > > >
> > > > > Hello, I would like to ask, how you handled this regression? Have you
> > > > > taken this my fix to address it? Or is it going to be addresses in other
> > > > > way?
> > > >
> > > > It's already fixed in cifs-utils-7.2 by commit 8b4b6e459d2a
> > > > ("getcifsacl: fix return code check for getting full ACL").
> > >
> > > Ok, and into kernel is not going to be addressed that regression for
> > > older cifs-utils?
> >
> > I thought we had decided that it was risky to intentionally return the
> > wrong return code to
> > userspace, to workaround an app bug (especially since it is easier to update
> > cifs-utils than update the kernel, and also since cifs-utils update to
> > 7.3 is strongly
> > encouraged for users due to multiple security issues fixed)
> >
> > --
> > Thanks,
> >
> > Steve
>
> Ok, fine. I just wanted to be sure that we have not forgot for something.



-- 
Thanks,

Steve