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
| ||
|
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
Powered by blists - more mailing lists