Bug in atmel-ecc driver

[Uwe Kleine-König]

Hi,

On Tue, May 17, 2022 at 01:11:22PM +0000, Tudor.Ambarus at microchip.com wrote:
> On 5/17/22 13:24, Uwe Kleine-König wrote:
> > On Fri, May 13, 2022 at 03:59:54PM +0200, Uwe Kleine-König wrote:
> >> TL;DR: when a device bound to the drivers/crypto/atmel-ecc.c driver is
> >> unbound while tfm_count isn't zero, this probably results in a
> >> use-after-free.
> >>
> >> The .remove function has:
> >>
> >> 	if (atomic_read(&i2c_priv->tfm_count)) {
> >>                 dev_err(&client->dev, "Device is busy\n");
> >>                 return -EBUSY;
> >>         }
> >>
> >> before actually calling the cleanup stuff. If this branch is hit the
> >> result is likely:
> >>
> >>  - "Device is busy" from drivers/crypto/atmel-ecc.c
> >>  - "remove failed (EBUSY), will be ignored" from the i2c core
> >>  - the devm cleanup callbacks are called, including the one kfreeing
> >>    *i2c_priv
> >>  - at a later time atmel_ecc_i2c_client_free() is called which does
> >>    atomic_dec(&i2c_priv->tfm_count);
> >>  - *boom*
> >>
> >> I think to fix that you need to call get_device for the i2c device
> >> before increasing tfm_count (and a matching put_device when decreasing
> >> it). Having said that the architecture of this driver looks strange to
> >> me, so there might be nicer fixes (probably with more effort).
> > I tried to understand the architecture a bit, what I found is
> > irritating. So the atmel-ecc driver provides a static struct kpp_alg
> > atmel_ecdh_nist_p256 which embeds a struct crypto_alg (.base). During
> > .probe() it calls crypto_register_kpp on that global kpp_alg. That is,
> > if there are two or more devices bound to this driver, the same kpp_alg
> > structure is registered repeatedly.  This involves (among others)
> > 
> >  - refcount_set(&atmel_ecdh_nist_p256.base.cra_refcount)
> >    in crypto_check_alg()
> >  - INIT_LIST_HEAD(&atmel_ecdh_nist_p256.base.cra_users)
> >    in __crypto_register_alg()
> > 
> > and then a check about registering the same alg twice which makes the
> > call crypto_register_alg() return -EEXIST. So if a second device is
> > bound, it probably corrupts the first device and then fails to probe.
> > 
> > So there can always be (at most) only one bound device which somehow
> > makes the whole logic in atmel_ecdh_init_tfm ->
> > atmel_ecc_i2c_client_alloc to select the least used(?) i2c client among
> > all the bound devices ridiculous.
> 
> It's been a while since I last worked with ateccx08, but as far as I remember
> it contains 3 crypto IPs (ecdh, ecdsa, sha) that communicate over the same
> i2c address. So if someone adds support for all algs and plug in multiple
> ateccx08 devices, then the distribution of tfms across the i2c clients may work.

It would require to register the crypto backends independent of the
.probe() routine though.

> Anyway, if you feel that the complexity is superfluous as the code is now, we
> can get rid of the i2c_client_alloc logic and add it later on when/if needed.

If it's you who acts, do whatever pleases you. If it's me I'd go for a
quick and simple solution to get back to what I originally want to do
with this driver.

So I'd go for something like

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 333fbefbbccb..e7f3f4793c55 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -349,8 +349,13 @@ static int atmel_ecc_remove(struct i2c_client *client)
 
 	/* Return EBUSY if i2c client already allocated. */
 	if (atomic_read(&i2c_priv->tfm_count)) {
-		dev_err(&client->dev, "Device is busy\n");
-		return -EBUSY;
+		/*
+		 * After we return here, the memory backing the device is freed.
+		 * If there is still some action pending, it probably involves
+		 * accessing free'd memory.
+		 */
+		dev_emerg(&client->dev, "Hell is about to break loose, expect memory corruption.\n");
+		return 0;
 	}
 
 	crypto_unregister_kpp(&atmel_ecdh_nist_p256);

because I'm not in yacc-shaving mood.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20220517/f8ec8a87/attachment-0001.sig>