Bug in atmel-ecc driver

[Tudor.Ambarus at microchip.com]

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 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.

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