Re: [EXTERNAL] re: cifs: Retain old ACEs when converting between mode bits and ACL.
From: Shyam Prasad N
Date: Wed Feb 24 2021 - 11:59:21 EST
Hi Steve,
Please accept this fix for the bug that Colin pointed out.
This can be hit if the server has a corrupted SD, or it got corrupted
over the network.
We used to ignore the ACL in such a case (which in combination with my
patches caused the issue). But I think we should be returning an error
immediately.
Regards,
Shyam
On Wed, Feb 24, 2021 at 7:16 AM Shyam Prasad <Shyam.Prasad@xxxxxxxxxxxxx> wrote:
>
>
Hi Colin,
>
>
Thanks for reporting this. I'll submit a fix.
>
>
Regards,
>
Shyam
>
>
-----Original Message-----
>
From: Colin Ian King <colin.king@xxxxxxxxxxxxx>
>
Sent: Wednesday, February 24, 2021 6:14 PM
>
To: Shyam Prasad <Shyam.Prasad@xxxxxxxxxxxxx>
>
Cc: Steve French <sfrench@xxxxxxxxx>; linux-cifs@xxxxxxxxxxxxxxx; samba-technical@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>
Subject: [EXTERNAL] re: cifs: Retain old ACEs when converting between mode bits and ACL.
>
>
Hi,
>
>
Static analysis on linux-next with Coverity had detected a potential null pointer dereference with the following commit:
>
>
commit f5065508897a922327f32223082325d10b069ebc
>
Author: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
>
Date: Fri Feb 12 04:38:43 2021 -0800
>
>
cifs: Retain old ACEs when converting between mode bits and ACL.
>
>
The analysis is as follows:
>
>
1258 /* Convert permission bits from mode to equivalent CIFS ACL */
>
1259 static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
>
1260 __u32 secdesclen, __u32 *pnsecdesclen, __u64 *pnmode, kuid_t
>
uid, kgid_t gid,
>
1261 bool mode_from_sid, bool id_from_sid, int *aclflag)
>
1262 {
>
1263 int rc = 0;
>
1264 __u32 dacloffset;
>
1265 __u32 ndacloffset;
>
1266 __u32 sidsoffset;
>
1267 struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
>
1268 struct cifs_sid *nowner_sid_ptr = NULL, *ngroup_sid_ptr = NULL;
>
>
1. assign_zero: Assigning: dacl_ptr = NULL.
>
>
1269 struct cifs_acl *dacl_ptr = NULL; /* no need for SACL ptr */
>
1270 struct cifs_acl *ndacl_ptr = NULL; /* no need for SACL ptr */
>
1271 char *end_of_acl = ((char *)pntsd) + secdesclen;
>
1272 u16 size = 0;
>
1273
>
1274 dacloffset = le32_to_cpu(pntsd->dacloffset);
>
>
2. Condition dacloffset, taking false branch.
>
>
1275 if (dacloffset) {
>
1276 dacl_ptr = (struct cifs_acl *)((char *)pntsd +
>
dacloffset);
>
1277 if (end_of_acl < (char *)dacl_ptr +
>
le16_to_cpu(dacl_ptr->size)) {
>
1278 cifs_dbg(VFS, "Existing ACL size is wrong.
>
Discarding old ACL\n");
>
1279 dacl_ptr = NULL;
>
>
NOTE: dacl_ptr is set to NULL and dacloffset is true
>
>
1280 }
>
1281 }
>
1282
>
1283 owner_sid_ptr = (struct cifs_sid *)((char *)pntsd +
>
1284 le32_to_cpu(pntsd->osidoffset));
>
1285 group_sid_ptr = (struct cifs_sid *)((char *)pntsd +
>
1286 le32_to_cpu(pntsd->gsidoffset));
>
1287
>
>
3. Condition pnmode, taking true branch.
>
4. Condition *pnmode != 18446744073709551615ULL, taking false branch.
>
>
1288 if (pnmode && *pnmode != NO_CHANGE_64) { /* chmod */
>
1289 ndacloffset = sizeof(struct cifs_ntsd);
>
1290 ndacl_ptr = (struct cifs_acl *)((char *)pnntsd +
>
ndacloffset);
>
1291 ndacl_ptr->revision =
>
1292 dacloffset ? dacl_ptr->revision :
>
cpu_to_le16(ACL_REVISION);
>
1293
>
1294 ndacl_ptr->size = cpu_to_le16(0);
>
1295 ndacl_ptr->num_aces = cpu_to_le32(0);
>
1296
>
1297 rc = set_chmod_dacl(dacl_ptr, ndacl_ptr,
>
owner_sid_ptr, group_sid_ptr,
>
1298 pnmode, mode_from_sid);
>
1299
>
1300 sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
>
1301 /* copy the non-dacl portion of secdesc */
>
1302 *pnsecdesclen = copy_sec_desc(pntsd, pnntsd, sidsoffset,
>
1303 NULL, NULL);
>
1304
>
1305 *aclflag |= CIFS_ACL_DACL;
>
1306 } else {
>
1307 ndacloffset = sizeof(struct cifs_ntsd);
>
1308 ndacl_ptr = (struct cifs_acl *)((char *)pnntsd +
>
ndacloffset);
>
>
5. Condition dacloffset, taking false branch.
>
>
1309 ndacl_ptr->revision =
>
1310 dacloffset ? dacl_ptr->revision :
>
cpu_to_le16(ACL_REVISION);
>
>
Explicit null dereferenced (FORWARD_NULL)
>
>
6. var_deref_op: Dereferencing null pointer dacl_ptr.
>
>
1311 ndacl_ptr->num_aces = dacl_ptr->num_aces;
>
>
>
Line 1309..1311, when dacloffset and dacl_ptr is null we hit a null ptr dereference on dacl_ptr.
>
--
Regards,
Shyam
Attachment:
0001-cifs-If-a-corrupted-DACL-is-returned-by-the-server-b.patch
Description: Binary data