Hi,
On Tue, May 17, 2022 at 01:11:22PM +0000, Tudor.Ambarus@xxxxxxxxxxxxx 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/ |
Attachment:
signature.asc
Description: PGP signature
