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@xxxxxxxxxx>
AuthorDate: Fri Oct 28 12:45:10 2022 +0200
Commit: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
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,
