2021-03-18 20:53 GMT+09:00, Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
> Hello Namjae Jeon,
Hi Dan,
First, Thanks so much for your detailed explanation.
I will fix them and apply the patch included your reported-by tag tomorrow.
If you find any other issues, Let me know it:)
Thanks!
>
> 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
>
