netdev - 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]

Date:   Tue, 30 May 2017 22:29:53 -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 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
>> 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