On 5/17/22 17:33, Uwe Kleine-König wrote:
>
> Hi,
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.
would be good to explain why i2c core will ignore -EBUSY.
I can't allocate time for this right now, so if you're in a hurry, it's fine
by me.
> + */
> + 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
