[bug report] cifsd: make xattr format of ksmbd compatible with samba's one

[Dan Carpenter <dan.carpenter@oracle.com>]

Hello Namjae Jeon,

The patch affbd69c2cb5: "cifsd: make xattr format of ksmbd compatible
with samba's one" from Jan 28, 2021, leads to the following static
checker warning:

	fs/cifsd/smbacl.c:1140 smb_check_perm_dacl()
	error: we previously assumed 'pntsd' could be null (see line 1137)

fs/cifsd/smbacl.c
  1119  int smb_check_perm_dacl(struct ksmbd_conn *conn, struct dentry *dentry,
  1120                  __le32 *pdaccess, int uid)
  1121  {
  1122          struct smb_ntsd *pntsd = NULL;
  1123          struct smb_acl *pdacl;
  1124          struct posix_acl *posix_acls;
  1125          int rc = 0, acl_size;
  1126          struct smb_sid sid;
  1127          int granted = le32_to_cpu(*pdaccess & ~FILE_MAXIMAL_ACCESS_LE);
  1128          struct smb_ace *ace;
  1129          int i, found = 0;
  1130          unsigned int access_bits = 0;
  1131          struct smb_ace *others_ace = NULL;
  1132          struct posix_acl_entry *pa_entry;
  1133          unsigned int sid_type = SIDOWNER;
  1134  
  1135          ksmbd_debug(SMB, "check permission using windows acl\n");
  1136          acl_size = ksmbd_vfs_get_sd_xattr(conn, dentry, &pntsd);
  1137          if (acl_size <= 0 || (pntsd && !pntsd->dacloffset))
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Presumably this should be "if (acl_size <= 0 || !pntsd)
				return 0;

If pntsd is NULL then we are toasted.

  1138                  return 0;
  1139  
  1140          pdacl = (struct smb_acl *)((char *)pntsd + le32_to_cpu(pntsd->dacloffset));

But as I look at this warning, I am much more concerned that we are
trusting pntsd->dacloffset at all.  It doesn't appear that we have
bounds checked the upper limit.

Unrelated and minor:  The ndr_read_bytes() function has the src, dest
parameters reversed which is going to chaos and confusion and in the
future.  ;)

  1141          if (!pdacl->num_aces) {
  1142                  if (!(le16_to_cpu(pdacl->size) - sizeof(struct smb_acl)) &&
  1143                      *pdaccess & ~(FILE_READ_CONTROL_LE | FILE_WRITE_DAC_LE)) {
  1144                          rc = -EACCES;
  1145                          goto err_out;
  1146                  }
  1147                  kfree(pntsd);
  1148                  return 0;
  1149          }

There is another similar sort of static checker warning:

fs/cifsd/smbacl.c:803 parse_sec_desc() warn: 'dacl_ptr' can't be NULL
   778  int parse_sec_desc(struct smb_ntsd *pntsd, int acl_len,
   779                  struct smb_fattr *fattr)
   780  {
   781          int rc = 0;
   782          struct smb_sid *owner_sid_ptr, *group_sid_ptr;
   783          struct smb_acl *dacl_ptr; /* no need for SACL ptr */
   784          char *end_of_acl = ((char *)pntsd) + acl_len;
   785          __u32 dacloffset;
   786          int total_ace_size = 0, pntsd_type;
   787  
   788          if (pntsd == NULL)
   789                  return -EIO;
   790  
   791          owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
   792                          le32_to_cpu(pntsd->osidoffset));
   793          group_sid_ptr = (struct smb_sid *)((char *)pntsd +
   794                          le32_to_cpu(pntsd->gsidoffset));
   795          dacloffset = le32_to_cpu(pntsd->dacloffset);
   796          dacl_ptr = (struct smb_acl *)((char *)pntsd + dacloffset);
                                                      ^^^^^
pntsd is non-NULL so dacl_ptr can't be NULL.  We're trusting a lot of
these offsets to be non-malicious.

   797          ksmbd_debug(SMB,
   798                  "revision %d type 0x%x ooffset 0x%x goffset 0x%x sacloffset 0x%x dacloffset 0x%x\n",
   799                   pntsd->revision, pntsd->type, le32_to_cpu(pntsd->osidoffset),
   800                   le32_to_cpu(pntsd->gsidoffset),
   801                   le32_to_cpu(pntsd->sacloffset), dacloffset);
   802  
   803          if (dacloffset && dacl_ptr)
   804                  total_ace_size =
   805                          le16_to_cpu(dacl_ptr->size) - sizeof(struct smb_acl);
   806  
   807          pntsd_type = le16_to_cpu(pntsd->type);
   808  

regards,
dan carpenter