Hi Colin,
Thanks for reporting this. I'll submit a fix.
Regards,
Shyam
-----Original Message-----
From: Colin Ian King <[email protected]>
Sent: Wednesday, February 24, 2021 6:14 PM
To: Shyam Prasad <[email protected]>
Cc: Steve French <[email protected]>; [email protected]; [email protected]; [email protected]
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 <[email protected]>
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.
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 <[email protected]> wrote:
>
> Hi Colin,
>
> Thanks for reporting this. I'll submit a fix.
>
> Regards,
> Shyam
>
> -----Original Message-----
> From: Colin Ian King <[email protected]>
> Sent: Wednesday, February 24, 2021 6:14 PM
> To: Shyam Prasad <[email protected]>
> Cc: Steve French <[email protected]>; [email protected]; [email protected]; [email protected]
> 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 <[email protected]>
> 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
Add the RB from Rohith and merged into cifs-2.6.git for-next
On Wed, Feb 24, 2021 at 10:58 AM Shyam Prasad N via samba-technical
<[email protected]> wrote:
>
> 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 <[email protected]> wrote:
> >
> > Hi Colin,
> >
> > Thanks for reporting this. I'll submit a fix.
> >
> > Regards,
> > Shyam
> >
> > -----Original Message-----
> > From: Colin Ian King <[email protected]>
> > Sent: Wednesday, February 24, 2021 6:14 PM
> > To: Shyam Prasad <[email protected]>
> > Cc: Steve French <[email protected]>; [email protected]; [email protected]; [email protected]
> > 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 <[email protected]>
> > 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
--
Thanks,
Steve