* [bug report] cifsd: make xattr format of ksmbd compatible with samba's one
@ 2021-03-18 11:53 Dan Carpenter
2021-03-18 12:44 ` Namjae Jeon
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2021-03-18 11:53 UTC (permalink / raw)
To: namjae.jeon; +Cc: linux-cifs
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] cifsd: make xattr format of ksmbd compatible with samba's one
2021-03-18 11:53 [bug report] cifsd: make xattr format of ksmbd compatible with samba's one Dan Carpenter
@ 2021-03-18 12:44 ` Namjae Jeon
0 siblings, 0 replies; 2+ messages in thread
From: Namjae Jeon @ 2021-03-18 12:44 UTC (permalink / raw)
To: Dan Carpenter; +Cc: namjae.jeon, linux-cifs
2021-03-18 20:53 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>:
> 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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-03-18 12:45 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18 11:53 [bug report] cifsd: make xattr format of ksmbd compatible with samba's one Dan Carpenter
2021-03-18 12:44 ` Namjae Jeon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).