Bug in atmel-ecc driver

Bug in atmel-ecc driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]

Hello,

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 noticed this issue while working on my quest to make i2c-remove
callbacks return void. So if you address this, it would be great if you
did that in a way that makes atmel_ecc_remove always return 0. 

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Bug in atmel-ecc driver

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1416 bytes --]

Hello,

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 noticed this issue while working on my quest to make i2c-remove
callbacks return void. So if you address this, it would be great if you
did that in a way that makes atmel_ecc_remove always return 0. 

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Bug in atmel-ecc driver

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2890 bytes --]

Hello,

[added Ard to Cc as he last touched that driver]

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.

Is there someone still caring for this driver? Does this justify 

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 7b2d138bc83e..b3242fba82aa 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -573,6 +573,7 @@ config CRYPTO_DEV_ATMEL_I2C
 
 config CRYPTO_DEV_ATMEL_ECC
 	tristate "Support for Microchip / Atmel ECC hw accelerator"
+	depends on BROKEN
 	depends on I2C
 	select CRYPTO_DEV_ATMEL_I2C
 	select CRYPTO_ECDH

?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Bug in atmel-ecc driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2890 bytes --]

Hello,

[added Ard to Cc as he last touched that driver]

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.

Is there someone still caring for this driver? Does this justify 

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 7b2d138bc83e..b3242fba82aa 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -573,6 +573,7 @@ config CRYPTO_DEV_ATMEL_I2C
 
 config CRYPTO_DEV_ATMEL_ECC
 	tristate "Support for Microchip / Atmel ECC hw accelerator"
+	depends on BROKEN
 	depends on I2C
 	select CRYPTO_DEV_ATMEL_I2C
 	select CRYPTO_ECDH

?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Bug in atmel-ecc driver

[Tudor.Ambarus]

On 5/17/22 13:24, Uwe Kleine-König wrote:
> Hello,
> 
Hi, Uwe,

> [added Ard to Cc as he last touched that driver]
> 
> 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.
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.

Cheers,
ta
> 
> Is there someone still caring for this driver? Does this justify 
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 7b2d138bc83e..b3242fba82aa 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -573,6 +573,7 @@ config CRYPTO_DEV_ATMEL_I2C
>  
>  config CRYPTO_DEV_ATMEL_ECC
>  	tristate "Support for Microchip / Atmel ECC hw accelerator"
> +	depends on BROKEN
>  	depends on I2C
>  	select CRYPTO_DEV_ATMEL_I2C
>  	select CRYPTO_ECDH
> 
> ?
> 
> Best regards
> Uwe

Re: Bug in atmel-ecc driver

[Tudor.Ambarus]

On 5/17/22 13:24, Uwe Kleine-König wrote:
> Hello,
> 
Hi, Uwe,

> [added Ard to Cc as he last touched that driver]
> 
> 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.
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.

Cheers,
ta
> 
> Is there someone still caring for this driver? Does this justify 
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 7b2d138bc83e..b3242fba82aa 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -573,6 +573,7 @@ config CRYPTO_DEV_ATMEL_I2C
>  
>  config CRYPTO_DEV_ATMEL_ECC
>  	tristate "Support for Microchip / Atmel ECC hw accelerator"
> +	depends on BROKEN
>  	depends on I2C
>  	select CRYPTO_DEV_ATMEL_I2C
>  	select CRYPTO_ECDH
> 
> ?
> 
> Best regards
> Uwe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Bug in atmel-ecc driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4229 bytes --]

Hi,

On Tue, May 17, 2022 at 01:11:22PM +0000, Tudor.Ambarus@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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Bug in atmel-ecc driver

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 4229 bytes --]

Hi,

On Tue, May 17, 2022 at 01:11:22PM +0000, Tudor.Ambarus@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/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Bug in atmel-ecc driver

[Tudor.Ambarus]

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

Re: Bug in atmel-ecc driver

[Tudor.Ambarus]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Bug in atmel-ecc driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 6001 bytes --]

Hello,

[fixed Ard's email address]

On Wed, May 18, 2022 at 10:07:32AM +0000, Tudor.Ambarus@microchip.com wrote:
> On 5/17/22 17:33, Uwe Kleine-König wrote:
> > On Tue, May 17, 2022 at 01:11:22PM +0000, Tudor.Ambarus@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.

In general it's impossible to do error handling (e.g. retry calling the
remove callback) because the device that is removed might be physically
removed. That mostly doesn't apply to i2c, but that's how the device
model works. So if you look into the i2c core: The remove callback is
called from i2c_device_remove() which is the remove callback for the i2c
bus. It's prototype is:

	static void i2c_device_remove(struct device *dev)

so there is no way to pass an error to the device core layer.
Until fc7a6209d5710618eb4f72a77cd81b8d694ecf89 this wasn't so obvious
and many busses returned an int, that was however ignored by the driver
core. The quest here is to change the bus specific methods to return
void. So the eventual goal here is for i2c:

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fbda5ada2afc..066b541a0d5d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -273,7 +273,7 @@ struct i2c_driver {
 
 	/* Standard driver model interfaces */
 	int (*probe)(struct i2c_client *client, const struct i2c_device_id *id);
-	int (*remove)(struct i2c_client *client);
+	void (*remove)(struct i2c_client *client);
 
 	/* New driver model interface to aid the seamless removal of the
 	 * current probe()'s, more commonly unused than used second parameter.
 

to don't let i2c driver authors assume they can return an error value.

See

	a0386bba70934d42f586eaf68b21d5eeaffa7bd0
	b2c943e52705b211d1aa0633c9196150cf30be47
	15f83bb0191261adece5a26bfdf93c6eccdbc0bb

for a few more examples.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Bug in atmel-ecc driver

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 6001 bytes --]

Hello,

[fixed Ard's email address]

On Wed, May 18, 2022 at 10:07:32AM +0000, Tudor.Ambarus@microchip.com wrote:
> On 5/17/22 17:33, Uwe Kleine-König wrote:
> > On Tue, May 17, 2022 at 01:11:22PM +0000, Tudor.Ambarus@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.

In general it's impossible to do error handling (e.g. retry calling the
remove callback) because the device that is removed might be physically
removed. That mostly doesn't apply to i2c, but that's how the device
model works. So if you look into the i2c core: The remove callback is
called from i2c_device_remove() which is the remove callback for the i2c
bus. It's prototype is:

	static void i2c_device_remove(struct device *dev)

so there is no way to pass an error to the device core layer.
Until fc7a6209d5710618eb4f72a77cd81b8d694ecf89 this wasn't so obvious
and many busses returned an int, that was however ignored by the driver
core. The quest here is to change the bus specific methods to return
void. So the eventual goal here is for i2c:

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fbda5ada2afc..066b541a0d5d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -273,7 +273,7 @@ struct i2c_driver {
 
 	/* Standard driver model interfaces */
 	int (*probe)(struct i2c_client *client, const struct i2c_device_id *id);
-	int (*remove)(struct i2c_client *client);
+	void (*remove)(struct i2c_client *client);
 
 	/* New driver model interface to aid the seamless removal of the
 	 * current probe()'s, more commonly unused than used second parameter.
 

to don't let i2c driver authors assume they can return an error value.

See

	a0386bba70934d42f586eaf68b21d5eeaffa7bd0
	b2c943e52705b211d1aa0633c9196150cf30be47
	15f83bb0191261adece5a26bfdf93c6eccdbc0bb

for a few more examples.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Uwe Kleine-König]

Returning an error value in an i2c remove callback results in an error
message being emitted by the i2c core, but otherwise it doesn't make a
difference. The device goes away anyhow and the devm cleanups are
called.

As atmel_ecc_remove() already emits an error message on failure and the
additional error message by the i2c core doesn't add any useful
information, change the return value to zero to suppress this message.

Also make the error message a bit more drastical because when the device
is still busy on remove, it's likely that it will access freed memory
soon.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/crypto/atmel-ecc.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 333fbefbbccb..6ba38275de8c 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -349,8 +349,16 @@ 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.
+		 * That happens no matter what the return value of this function
+		 * is because in the Linux device model there is no error
+		 * handling for unbinding a driver.
+		 * If there is still some action pending, it probably involves
+		 * accessing the freed memory.
+		 */
+		dev_emerg(&client->dev, "Device is busy, expect memory corruption.\n");
+		return 0;
 	}
 
 	crypto_unregister_kpp(&atmel_ecdh_nist_p256);

base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.35.1

[PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Uwe Kleine-König]

Returning an error value in an i2c remove callback results in an error
message being emitted by the i2c core, but otherwise it doesn't make a
difference. The device goes away anyhow and the devm cleanups are
called.

As atmel_ecc_remove() already emits an error message on failure and the
additional error message by the i2c core doesn't add any useful
information, change the return value to zero to suppress this message.

Also make the error message a bit more drastical because when the device
is still busy on remove, it's likely that it will access freed memory
soon.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/crypto/atmel-ecc.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 333fbefbbccb..6ba38275de8c 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -349,8 +349,16 @@ 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.
+		 * That happens no matter what the return value of this function
+		 * is because in the Linux device model there is no error
+		 * handling for unbinding a driver.
+		 * If there is still some action pending, it probably involves
+		 * accessing the freed memory.
+		 */
+		dev_emerg(&client->dev, "Device is busy, expect memory corruption.\n");
+		return 0;
 	}
 
 	crypto_unregister_kpp(&atmel_ecdh_nist_p256);

base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

Hello,

On Fri, May 20, 2022 at 07:21:00PM +0200, Uwe Kleine-König wrote:
> Returning an error value in an i2c remove callback results in an error
> message being emitted by the i2c core, but otherwise it doesn't make a
> difference. The device goes away anyhow and the devm cleanups are
> called.
> 
> As atmel_ecc_remove() already emits an error message on failure and the
> additional error message by the i2c core doesn't add any useful
> information, change the return value to zero to suppress this message.
> 
> Also make the error message a bit more drastical because when the device
> is still busy on remove, it's likely that it will access freed memory
> soon.
> 
> This patch is a preparation for making i2c remove callbacks return void.

I want to tackle this (i.e.

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fbda5ada2afc..066b541a0d5d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -273,7 +273,7 @@ struct i2c_driver {

 	/* Standard driver model interfaces */
 	int (*probe)(struct i2c_client *client, const struct i2c_device_id *id);
-	int (*remove)(struct i2c_client *client);
+	void (*remove)(struct i2c_client *client);

 	/* New driver model interface to aid the seamless removal of the
 	 * current probe()'s, more commonly unused than used second parameter.

) directly after the next merge window. That is (depending on Linus's
counting capabilities) after v5.20-rc1. So I ask you to either take this
crypto patch before (my preferred option), or accept that I send it as part
of a bigger series that eventually contains the above hunk and will
probably be merged via the i2c tree.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1860 bytes --]

Hello,

On Fri, May 20, 2022 at 07:21:00PM +0200, Uwe Kleine-König wrote:
> Returning an error value in an i2c remove callback results in an error
> message being emitted by the i2c core, but otherwise it doesn't make a
> difference. The device goes away anyhow and the devm cleanups are
> called.
> 
> As atmel_ecc_remove() already emits an error message on failure and the
> additional error message by the i2c core doesn't add any useful
> information, change the return value to zero to suppress this message.
> 
> Also make the error message a bit more drastical because when the device
> is still busy on remove, it's likely that it will access freed memory
> soon.
> 
> This patch is a preparation for making i2c remove callbacks return void.

I want to tackle this (i.e.

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fbda5ada2afc..066b541a0d5d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -273,7 +273,7 @@ struct i2c_driver {

 	/* Standard driver model interfaces */
 	int (*probe)(struct i2c_client *client, const struct i2c_device_id *id);
-	int (*remove)(struct i2c_client *client);
+	void (*remove)(struct i2c_client *client);

 	/* New driver model interface to aid the seamless removal of the
 	 * current probe()'s, more commonly unused than used second parameter.

) directly after the next merge window. That is (depending on Linus's
counting capabilities) after v5.20-rc1. So I ask you to either take this
crypto patch before (my preferred option), or accept that I send it as part
of a bigger series that eventually contains the above hunk and will
probably be merged via the i2c tree.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Tudor.Ambarus]

On 5/20/22 20:21, Uwe Kleine-König wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Returning an error value in an i2c remove callback results in an error
> message being emitted by the i2c core, but otherwise it doesn't make a
> difference. The device goes away anyhow and the devm cleanups are
> called.
> 
> As atmel_ecc_remove() already emits an error message on failure and the
> additional error message by the i2c core doesn't add any useful
> information, change the return value to zero to suppress this message.
> 
> Also make the error message a bit more drastical because when the device
> is still busy on remove, it's likely that it will access freed memory
> soon.
> 
> This patch is a preparation for making i2c remove callbacks return void.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/crypto/atmel-ecc.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> index 333fbefbbccb..6ba38275de8c 100644
> --- a/drivers/crypto/atmel-ecc.c
> +++ b/drivers/crypto/atmel-ecc.c
> @@ -349,8 +349,16 @@ 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.
> +                * That happens no matter what the return value of this function
> +                * is because in the Linux device model there is no error
> +                * handling for unbinding a driver.
> +                * If there is still some action pending, it probably involves
> +                * accessing the freed memory.
> +                */
> +               dev_emerg(&client->dev, "Device is busy, expect memory corruption.\n");
> +               return 0;
>         }
> 
>         crypto_unregister_kpp(&atmel_ecdh_nist_p256);
> 
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17
> --
> 2.35.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Tudor.Ambarus]

On 5/20/22 20:21, Uwe Kleine-König wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Returning an error value in an i2c remove callback results in an error
> message being emitted by the i2c core, but otherwise it doesn't make a
> difference. The device goes away anyhow and the devm cleanups are
> called.
> 
> As atmel_ecc_remove() already emits an error message on failure and the
> additional error message by the i2c core doesn't add any useful
> information, change the return value to zero to suppress this message.
> 
> Also make the error message a bit more drastical because when the device
> is still busy on remove, it's likely that it will access freed memory
> soon.
> 
> This patch is a preparation for making i2c remove callbacks return void.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/crypto/atmel-ecc.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> index 333fbefbbccb..6ba38275de8c 100644
> --- a/drivers/crypto/atmel-ecc.c
> +++ b/drivers/crypto/atmel-ecc.c
> @@ -349,8 +349,16 @@ 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.
> +                * That happens no matter what the return value of this function
> +                * is because in the Linux device model there is no error
> +                * handling for unbinding a driver.
> +                * If there is still some action pending, it probably involves
> +                * accessing the freed memory.
> +                */
> +               dev_emerg(&client->dev, "Device is busy, expect memory corruption.\n");
> +               return 0;
>         }
> 
>         crypto_unregister_kpp(&atmel_ecdh_nist_p256);
> 
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17
> --
> 2.35.1
> 

Re: [PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1469 bytes --]

Hello

On Wed, Jun 08, 2022 at 04:33:48AM +0000, Tudor.Ambarus@microchip.com wrote:
> On 5/20/22 20:21, Uwe Kleine-König wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Returning an error value in an i2c remove callback results in an error
> > message being emitted by the i2c core, but otherwise it doesn't make a
> > difference. The device goes away anyhow and the devm cleanups are
> > called.
> > 
> > As atmel_ecc_remove() already emits an error message on failure and the
> > additional error message by the i2c core doesn't add any useful
> > information, change the return value to zero to suppress this message.
> > 
> > Also make the error message a bit more drastical because when the device
> > is still busy on remove, it's likely that it will access freed memory
> > soon.
> > 
> > This patch is a preparation for making i2c remove callbacks return void.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

In the past patches were picked up by Herbert. I assume your R-b tag was
the missing bit to make him pick up this patch? To make a bit more sure
that will happen, I added him and davem to Cc.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

Hello

On Wed, Jun 08, 2022 at 04:33:48AM +0000, Tudor.Ambarus@microchip.com wrote:
> On 5/20/22 20:21, Uwe Kleine-König wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Returning an error value in an i2c remove callback results in an error
> > message being emitted by the i2c core, but otherwise it doesn't make a
> > difference. The device goes away anyhow and the devm cleanups are
> > called.
> > 
> > As atmel_ecc_remove() already emits an error message on failure and the
> > additional error message by the i2c core doesn't add any useful
> > information, change the return value to zero to suppress this message.
> > 
> > Also make the error message a bit more drastical because when the device
> > is still busy on remove, it's likely that it will access freed memory
> > soon.
> > 
> > This patch is a preparation for making i2c remove callbacks return void.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

In the past patches were picked up by Herbert. I assume your R-b tag was
the missing bit to make him pick up this patch? To make a bit more sure
that will happen, I added him and davem to Cc.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Tudor.Ambarus]

On 6/8/22 10:04, Uwe Kleine-König wrote:
> Hello
> 

Hi,

> On Wed, Jun 08, 2022 at 04:33:48AM +0000, Tudor.Ambarus@microchip.com wrote:
>> On 5/20/22 20:21, Uwe Kleine-König wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Returning an error value in an i2c remove callback results in an error
>>> message being emitted by the i2c core, but otherwise it doesn't make a
>>> difference. The device goes away anyhow and the devm cleanups are
>>> called.
>>>
>>> As atmel_ecc_remove() already emits an error message on failure and the
>>> additional error message by the i2c core doesn't add any useful
>>> information, change the return value to zero to suppress this message.
>>>
>>> Also make the error message a bit more drastical because when the device
>>> is still busy on remove, it's likely that it will access freed memory
>>> soon.
>>>
>>> This patch is a preparation for making i2c remove callbacks return void.
>>>
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> In the past patches were picked up by Herbert. I assume your R-b tag was

he still does.

> the missing bit to make him pick up this patch? To make a bit more sure

probably not.

> that will happen, I added him and davem to Cc.

Herbert usually queues patches in a two week time frame. Let's wait for him.

Cheers,
ta
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Tudor.Ambarus]

On 6/8/22 10:04, Uwe Kleine-König wrote:
> Hello
> 

Hi,

> On Wed, Jun 08, 2022 at 04:33:48AM +0000, Tudor.Ambarus@microchip.com wrote:
>> On 5/20/22 20:21, Uwe Kleine-König wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Returning an error value in an i2c remove callback results in an error
>>> message being emitted by the i2c core, but otherwise it doesn't make a
>>> difference. The device goes away anyhow and the devm cleanups are
>>> called.
>>>
>>> As atmel_ecc_remove() already emits an error message on failure and the
>>> additional error message by the i2c core doesn't add any useful
>>> information, change the return value to zero to suppress this message.
>>>
>>> Also make the error message a bit more drastical because when the device
>>> is still busy on remove, it's likely that it will access freed memory
>>> soon.
>>>
>>> This patch is a preparation for making i2c remove callbacks return void.
>>>
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> In the past patches were picked up by Herbert. I assume your R-b tag was

he still does.

> the missing bit to make him pick up this patch? To make a bit more sure

probably not.

> that will happen, I added him and davem to Cc.

Herbert usually queues patches in a two week time frame. Let's wait for him.

Cheers,
ta

Re: [PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Herbert Xu]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> Returning an error value in an i2c remove callback results in an error
> message being emitted by the i2c core, but otherwise it doesn't make a
> difference. The device goes away anyhow and the devm cleanups are
> called.
> 
> As atmel_ecc_remove() already emits an error message on failure and the
> additional error message by the i2c core doesn't add any useful
> information, change the return value to zero to suppress this message.
> 
> Also make the error message a bit more drastical because when the device
> is still busy on remove, it's likely that it will access freed memory
> soon.
> 
> This patch is a preparation for making i2c remove callbacks return void.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> drivers/crypto/atmel-ecc.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: atmel-ecc - Remove duplicated error reporting in .remove()

[Herbert Xu]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> Returning an error value in an i2c remove callback results in an error
> message being emitted by the i2c core, but otherwise it doesn't make a
> difference. The device goes away anyhow and the devm cleanups are
> called.
> 
> As atmel_ecc_remove() already emits an error message on failure and the
> additional error message by the i2c core doesn't add any useful
> information, change the return value to zero to suppress this message.
> 
> Also make the error message a bit more drastical because when the device
> is still busy on remove, it's likely that it will access freed memory
> soon.
> 
> This patch is a preparation for making i2c remove callbacks return void.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> drivers/crypto/atmel-ecc.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel