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
