Hello everybody,
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'm trying to figure out if this is a false positive or something that
needs to be fixed somehow.
I'd really appreciate any comment on this.
Thank you!
--
Gustavo A. R. Silva
Hi Marcel,
Quoting "Gustavo A. R. Silva" <[email protected]>:
> Hi Marcel,
>
> Quoting Marcel Holtmann <[email protected]>:
>
>> 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
Hi Marcel,
Quoting Marcel Holtmann <[email protected]>:
> 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
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;
}
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.
Maybe commenting this might be a good idea.
Regards
Marcel
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