linux-kernel - Re: [net-bluetooth] question about potential null pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170530223629.Horde.l1icLUD-56lD-xqEDjo6CDT@gator4166.hostgator.com>
Date:   Tue, 30 May 2017 22:36:29 -0500
From:   "Gustavo A. R. Silva" <garsilva@...eddedor.com>
To:     Marcel Holtmann <marcel@...tmann.org>
Cc:     "Gustavo F. Padovan" <gustavo@...ovan.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        "open list:BLUETOOTH DRIVERS" <linux-bluetooth@...r.kernel.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-bluetooth] question about potential null pointer
 dereference

Hi Marcel,

Quoting "Gustavo A. R. Silva" <garsilva@...eddedor.com>:

> Hi Marcel,
>
> Quoting Marcel Holtmann <marcel@...tmann.org>:
>
>> Hi Gustavo,
>>
>>> While looking into Coverity ID 1357456 I ran into the following  
>>> piece of code at net/bluetooth/smp.c:166
>>>
>>> 166/* The following functions map to the LE SC SMP crypto functions
>>> 167 * AES-CMAC, f4, f5, f6, g2 and h6.
>>> 168 */
>>> 169
>>> 170static int aes_cmac(struct crypto_shash *tfm, const u8 k[16],  
>>> const u8 *m,
>>> 171                    size_t len, u8 mac[16])
>>> 172{
>>> 173        uint8_t tmp[16], mac_msb[16], msg_msb[CMAC_MSG_MAX];
>>> 174        SHASH_DESC_ON_STACK(desc, tfm);
>>> 175        int err;
>>> 176
>>> 177        if (len > CMAC_MSG_MAX)
>>> 178                return -EFBIG;
>>> 179
>>> 180        if (!tfm) {
>>> 181                BT_ERR("tfm %p", tfm);
>>> 182                return -EINVAL;
>>> 183        }
>>> 184

BTW, what do you think about removing the IF block above?

>>> 185        desc->tfm = tfm;
>>> 186        desc->flags = 0;
>>> 187
>>> 188        /* Swap key and message from LSB to MSB */
>>> 189        swap_buf(k, tmp, 16);
>>> 190        swap_buf(m, msg_msb, len);
>>> 191
>>> 192        SMP_DBG("msg (len %zu) %*phN", len, (int) len, m);
>>> 193        SMP_DBG("key %16phN", k);
>>> 194
>>> 195        err = crypto_shash_setkey(tfm, tmp, 16);
>>> 196        if (err) {
>>> 197                BT_ERR("cipher setkey failed: %d", err);
>>> 198                return err;
>>> 199        }
>>> 200
>>> 201        err = crypto_shash_digest(desc, msg_msb, len, mac_msb);
>>> 202        shash_desc_zero(desc);
>>> 203        if (err) {
>>> 204                BT_ERR("Hash computation error %d", err);
>>> 205                return err;
>>> 206        }
>>> 207
>>> 208        swap_buf(mac_msb, mac, 16);
>>> 209
>>> 210        SMP_DBG("mac %16phN", mac);
>>> 211
>>> 212        return 0;
>>> 213}
>>>
>>> The issue here is that line 180 implies that pointer tfm might be  
>>> NULL. If this is the case, there is a potential NULL pointer  
>>> dereference at line 174 once pointer tfm is indirectly  
>>> dereferenced inside macro SHASH_DESC_ON_STACK().
>>>
>>> My question is if there is any chance that pointer tfm maybe be  
>>> NULL when calling macro SHASH_DESC_ON_STACK()?
>>
>> I think the part you are after is this:
>>
>>        smp->tfm_cmac = crypto_alloc_shash("cmac(aes)", 0, 0);
>>        if (IS_ERR(smp->tfm_cmac)) {
>>                BT_ERR("Unable to create CMAC crypto context");
>>                crypto_free_cipher(smp->tfm_aes);
>>                kzfree(smp);
>>                return NULL;
>>        }
>>
>
> Yeah, this makes it all clear.
>
>> So the tfm_cmac is part of the smp structure. However if there is  
>> no cipher, we destroy the smp structure and essentially run without  
>> SMP support. So it can not really be called anyway.
>>
>
> What I take from this is that as a general rule, I should first try  
> to identify whether the code I'm debugging is reachable or not,  
> depending on the specific structures and variables I'm interested in.
>
>> Maybe commenting this might be a good idea.
>>
>
> Yep, it wouldn't hurt.
>
> In the meantime I will triage and document this as a false positive.
>
> Thank you very much for the clarification, Marcel,
> I really appreciate it.
> --
> Gustavo A. R. Silva

Thanks
--
Gustavo A. R. Silva