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?
what do you mean by this? >>>> 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. Patches are welcome :) Regards Marcel