Messages in this thread | | | Date | Tue, 30 May 2017 22:29:53 -0500 | From | "Gustavo A. R. Silva" <> | Subject | Re: [net-bluetooth] question about potential null pointer dereference |
| |
Hi Marcel,
Quoting Marcel Holtmann <marcel@holtmann.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
| |