Message-ID: <CANT5p=obm1EaZm+exAwjRdgrh1YV8bfMZWe-=nmwP7ExTWqX+Q@mail.gmail.com>
Date:   Wed, 24 Feb 2021 08:56:40 -0800
From:   Shyam Prasad N <nspmangalore@...il.com>
To:     Shyam Prasad <Shyam.Prasad@...rosoft.com>,
        rohiths msft <rohiths.msft@...il.com>
Cc:     Colin Ian King <colin.king@...onical.com>,
        Steve French <sfrench@...ba.org>,
        "linux-cifs@...r.kernel.org" <linux-cifs@...r.kernel.org>,
        "samba-technical@...ts.samba.org" <samba-technical@...ts.samba.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [EXTERNAL] re: cifs: Retain old ACEs when converting between mode
 bits and ACL.

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@...rosoft.com> wrote:
>
> Hi Colin,
>
> Thanks for reporting this. I'll submit a fix.
>
> Regards,
> Shyam
>
> -----Original Message-----
> From: Colin Ian King <colin.king@...onical.com>
> Sent: Wednesday, February 24, 2021 6:14 PM
> To: Shyam Prasad <Shyam.Prasad@...rosoft.com>
> Cc: Steve French <sfrench@...ba.org>; linux-cifs@...r.kernel.org; samba-technical@...ts.samba.org; linux-kernel@...r.kernel.org
> 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@...rosoft.com>
> 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

Download attachment "0001-cifs-If-a-corrupted-DACL-is-returned-by-the-server-b.patch" of type "application/octet-stream" (1340 bytes)

Powered by blists - more mailing lists