[bug report] cifs: implement set acl method

[bug report] cifs: implement set acl method

[Dan Carpenter]

Hello Christian Brauner,

This is a semi-automatic email about new static checker warnings.

The patch dc1af4c4b472: "cifs: implement set acl method" from Sep 22,
2022, leads to the following Smatch complaint:

    fs/cifs/cifsacl.c:1781 cifs_set_acl()
    warn: variable dereferenced before check 'acl' (see line 1773)

fs/cifs/cifsacl.c
  1772			returns as xattrs */
  1773		if (posix_acl_xattr_size(acl->a_count) > CIFSMaxBufSize) {
                                         ^^^
I looked at the callers and "acl" can definitely be NULL at this point.
I feel like it would be nice to check it earlier and goto out directly,
but I don't know what a NULL acl is for...

  1774			cifs_dbg(FYI, "size of EA value too large\n");
  1775			rc = -EOPNOTSUPP;
  1776			goto out;
  1777		}
  1778	
  1779		switch (type) {
  1780		case ACL_TYPE_ACCESS:
  1781			if (!acl)
                            ^^^^
Too late.  And later on there is another check as well.

  1782				goto out;
  1783			if (sb->s_flags & SB_POSIXACL)

regards,
dan carpenter

Re: [bug report] cifs: implement set acl method

[Christian Brauner]

On Fri, Oct 28, 2022 at 01:38:29PM +0300, Dan Carpenter wrote:
> Hello Christian Brauner,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch dc1af4c4b472: "cifs: implement set acl method" from Sep 22,
> 2022, leads to the following Smatch complaint:
> 
>     fs/cifs/cifsacl.c:1781 cifs_set_acl()
>     warn: variable dereferenced before check 'acl' (see line 1773)
> 
> fs/cifs/cifsacl.c
>   1772			returns as xattrs */
>   1773		if (posix_acl_xattr_size(acl->a_count) > CIFSMaxBufSize) {
>                                          ^^^
> I looked at the callers and "acl" can definitely be NULL at this point.
> I feel like it would be nice to check it earlier and goto out directly,
> but I don't know what a NULL acl is for...
> 
>   1774			cifs_dbg(FYI, "size of EA value too large\n");
>   1775			rc = -EOPNOTSUPP;
>   1776			goto out;
>   1777		}
>   1778	
>   1779		switch (type) {
>   1780		case ACL_TYPE_ACCESS:
>   1781			if (!acl)
>                             ^^^^
> Too late.  And later on there is another check as well.
> 
>   1782				goto out;
>   1783			if (sb->s_flags & SB_POSIXACL)
> 
> regards,
> dan carpenter

Thanks for the report, Dank. I added the following fix on top. If that
work out I'll likely fold it into the original commit though given that
we're very still pre -rc4:

commit cb2144d66b0b24fd1b880fc72678ba21ca414dab (HEAD -> fs.acl.rework)
Author:     Christian Brauner <brauner@kernel.org>
AuthorDate: Fri Oct 28 12:45:10 2022 +0200
Commit:     Christian Brauner (Microsoft) <brauner@kernel.org>
CommitDate: Fri Oct 28 12:45:10 2022 +0200

    cifs: check whether acl is valid early

    Dan reported that acl is dereferenced before being checked and this is a
    valid problem. Fix it be erroring out early instead of doing it later after
    we've already relied on acl to be a valid pointer.

    Fixes: dc1af4c4b472 ("cifs: implement set acl method")
    Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
    Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
index 6a9f03c882dc..c647f0d56518 100644
--- a/fs/cifs/cifsacl.c
+++ b/fs/cifs/cifsacl.c
@@ -1764,6 +1764,10 @@ int cifs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
                rc = PTR_ERR(full_path);
                goto out;
        }
+
+       if (!acl)
+               goto out;
+
        /* return dos attributes as pseudo xattr */
        /* return alt name if available as pseudo attr */

@@ -1778,8 +1782,6 @@ int cifs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,

        switch (type) {
        case ACL_TYPE_ACCESS:
-               if (!acl)
-                       goto out;
                if (sb->s_flags & SB_POSIXACL)
                        rc = cifs_do_set_acl(xid, pTcon, full_path, acl,
                                             ACL_TYPE_ACCESS,
@@ -1788,8 +1790,6 @@ int cifs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
                break;

        case ACL_TYPE_DEFAULT:
-               if (!acl)
-                       goto out;
                if (sb->s_flags & SB_POSIXACL)
                        rc = cifs_do_set_acl(xid, pTcon, full_path, acl,
                                             ACL_TYPE_DEFAULT,

Re: [bug report] cifs: implement set acl method

[Dan Carpenter]

On Fri, Oct 28, 2022 at 12:49:16PM +0200, Christian Brauner wrote:
> On Fri, Oct 28, 2022 at 01:38:29PM +0300, Dan Carpenter wrote:
> > 
> > regards,
> > dan carpenter
> 
> Thanks for the report, Dan. I added the following fix on top. If that
> work out I'll likely fold it into the original commit though given that
> we're very still pre -rc4:
> 

Sounds good.  Thanks!

regards,
dan carpenter