diff mbox series

nfs: we don't support removing system.nfs4_acl

Message ID 20210211185444.GA6048@fieldses.org (mailing list archive)
State New
Headers show
Series nfs: we don't support removing system.nfs4_acl | expand

Commit Message

J. Bruce Fields Feb. 11, 2021, 6:54 p.m. UTC
From: "J. Bruce Fields" <bfields@redhat.com>

The contents of the system.nfs4_acl xattr are literally just the
xdr-encoded ACL attribute.  That attribute starts with a 4-byte integer
representing the number of ACEs in the ACL.  So even a zero-ACE ACL will
be at least 4 bytes.

We've never actually bothered to sanity-check the ACL encoding that
userspace gives us.  The only problem that causes is that we return an
error that's probably wrong.  (The server will return BADXDR, which
we'll translate to EIO, when EINVAL would make more sense.)

It's not much a problem in practice since the standard utilities give us
well-formed XDR.  The one case we're likely to see from userspace in
practice is a set of a zero-length xattr since that's how

	removexattr(path, "system.nfs4_acl")

is implemented.  It's worth trying to give a better error for that case.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfs/nfs4proc.c | 6 ++++++
 1 file changed, 6 insertions(+)

On Mon, Feb 08, 2021 at 05:08:55PM -0500, bfields@fieldses.org wrote:
> On Mon, Feb 08, 2021 at 07:31:38PM +0000, Trond Myklebust wrote:
> > OK. So you're not really saying that the SETATTR has a zero length
> > body, but that the ACL attribute in this case has a zero length body,
> > whereas in the 'empty acl' case, it is supposed to have a body
> > containing a zero-length nfsace4<> array. Fair enough.
> 
> Yep!  I'll see if I can think of a helpful concise comment, and resend.

Oops, forgot about this, here you go.--b.

Comments

Murphy Zhou March 4, 2021, 2:30 a.m. UTC | #1
Hi,

On Fri, Feb 12, 2021 at 2:58 AM bfields@fieldses.org
<bfields@fieldses.org> wrote:
>
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> The contents of the system.nfs4_acl xattr are literally just the
> xdr-encoded ACL attribute.  That attribute starts with a 4-byte integer
> representing the number of ACEs in the ACL.  So even a zero-ACE ACL will
> be at least 4 bytes.
>
> We've never actually bothered to sanity-check the ACL encoding that
> userspace gives us.  The only problem that causes is that we return an
> error that's probably wrong.  (The server will return BADXDR, which
> we'll translate to EIO, when EINVAL would make more sense.)
>
> It's not much a problem in practice since the standard utilities give us
> well-formed XDR.  The one case we're likely to see from userspace in
> practice is a set of a zero-length xattr since that's how
>
>         removexattr(path, "system.nfs4_acl")
>
> is implemented.  It's worth trying to give a better error for that case.
>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfs/nfs4proc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> On Mon, Feb 08, 2021 at 05:08:55PM -0500, bfields@fieldses.org wrote:
> > On Mon, Feb 08, 2021 at 07:31:38PM +0000, Trond Myklebust wrote:
> > > OK. So you're not really saying that the SETATTR has a zero length
> > > body, but that the ACL attribute in this case has a zero length body,
> > > whereas in the 'empty acl' case, it is supposed to have a body
> > > containing a zero-length nfsace4<> array. Fair enough.
> >
> > Yep!  I'll see if I can think of a helpful concise comment, and resend.
>
> Oops, forgot about this, here you go.--b.
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 2f4679a62712..86e87f7d7686 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5895,6 +5895,12 @@ static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl
>         unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
>         int ret, i;
>
> +       /*
> +        * We don't support removing system.nfs4_acl, and even a
> +        * 0-length ACL needs at least 4 bytes for the number of ACEs:
> +        */
> +       if (buflen < 4)
> +               return -EINVAL;
>         if (!nfs4_server_supports_acls(server))
>                 return -EOPNOTSUPP;
>         if (npages > ARRAY_SIZE(pages))
> --
> 2.29.2
>

Has this queued up for the next RC ?


Thanks,
Anna Schumaker March 12, 2021, 3:43 p.m. UTC | #2
Hi Murphy,

On Wed, Mar 3, 2021 at 9:30 PM Murphy Zhou <jencce.kernel@gmail.com> wrote:
>
> Hi,
>
> On Fri, Feb 12, 2021 at 2:58 AM bfields@fieldses.org
> <bfields@fieldses.org> wrote:
> >
> > From: "J. Bruce Fields" <bfields@redhat.com>
> >
> > The contents of the system.nfs4_acl xattr are literally just the
> > xdr-encoded ACL attribute.  That attribute starts with a 4-byte integer
> > representing the number of ACEs in the ACL.  So even a zero-ACE ACL will
> > be at least 4 bytes.
> >
> > We've never actually bothered to sanity-check the ACL encoding that
> > userspace gives us.  The only problem that causes is that we return an
> > error that's probably wrong.  (The server will return BADXDR, which
> > we'll translate to EIO, when EINVAL would make more sense.)
> >
> > It's not much a problem in practice since the standard utilities give us
> > well-formed XDR.  The one case we're likely to see from userspace in
> > practice is a set of a zero-length xattr since that's how
> >
> >         removexattr(path, "system.nfs4_acl")
> >
> > is implemented.  It's worth trying to give a better error for that case.
> >
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  fs/nfs/nfs4proc.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > On Mon, Feb 08, 2021 at 05:08:55PM -0500, bfields@fieldses.org wrote:
> > > On Mon, Feb 08, 2021 at 07:31:38PM +0000, Trond Myklebust wrote:
> > > > OK. So you're not really saying that the SETATTR has a zero length
> > > > body, but that the ACL attribute in this case has a zero length body,
> > > > whereas in the 'empty acl' case, it is supposed to have a body
> > > > containing a zero-length nfsace4<> array. Fair enough.
> > >
> > > Yep!  I'll see if I can think of a helpful concise comment, and resend.
> >
> > Oops, forgot about this, here you go.--b.
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 2f4679a62712..86e87f7d7686 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -5895,6 +5895,12 @@ static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl
> >         unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
> >         int ret, i;
> >
> > +       /*
> > +        * We don't support removing system.nfs4_acl, and even a
> > +        * 0-length ACL needs at least 4 bytes for the number of ACEs:
> > +        */
> > +       if (buflen < 4)
> > +               return -EINVAL;
> >         if (!nfs4_server_supports_acls(server))
> >                 return -EOPNOTSUPP;
> >         if (npages > ARRAY_SIZE(pages))
> > --
> > 2.29.2
> >
>
> Has this queued up for the next RC ?

Yeah, I have this queued up for the next bugfixes pull request.

Anna
>
>
> Thanks,
diff mbox series

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 2f4679a62712..86e87f7d7686 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5895,6 +5895,12 @@  static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl
 	unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
 	int ret, i;
 
+	/*
+	 * We don't support removing system.nfs4_acl, and even a
+	 * 0-length ACL needs at least 4 bytes for the number of ACEs:
+	 */
+	if (buflen < 4)
+		return -EINVAL;
 	if (!nfs4_server_supports_acls(server))
 		return -EOPNOTSUPP;
 	if (npages > ARRAY_SIZE(pages))