[PATCH 0/2] media: ir-kbd-i2c: fix potential OOPS & minor cleanup

[PATCH 0/2] media: ir-kbd-i2c: fix potential OOPS & minor cleanup

[Wolfram Sang]

From: Wolfram Sang <wsa@the-dreams.de>

This series is part of a tree-wide movement to replace the I2C API call
'i2c_new_dummy' which returns NULL with its new counterpart returning an
ERRPTR.

It was manually converted and only build tested (by me and buildbot). A
small cleanup was added while working on this driver. Looking for
someone to ack/rev/test this series.

The series is based on v5.3-rc1. A branch (with some more stuff included) can
be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/new_dummy

Thanks!


Wolfram Sang (2):
  media: ir-kbd-i2c: prevent potential NULL pointer access
  media: ir-kbd-i2c: remove outdated comments

 drivers/media/i2c/ir-kbd-i2c.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

-- 
2.20.1

[PATCH 1/2] media: ir-kbd-i2c: prevent potential NULL pointer access

[Wolfram Sang]

i2c_new_dummy() can fail returning a NULL pointer. The code does not
bail out in this case and the returned pointer is blindly used. Convert
to devm_i2c_new_dummy_device() which returns an ERR_PTR and also bail
out when failing the validity check.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/media/i2c/ir-kbd-i2c.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
index 876d7587a1da..f46717052efc 100644
--- a/drivers/media/i2c/ir-kbd-i2c.c
+++ b/drivers/media/i2c/ir-kbd-i2c.c
@@ -885,9 +885,12 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	INIT_DELAYED_WORK(&ir->work, ir_work);
 
 	if (probe_tx) {
-		ir->tx_c = i2c_new_dummy(client->adapter, 0x70);
-		if (!ir->tx_c) {
+		ir->tx_c = devm_i2c_new_dummy_device(&client->dev,
+						     client->adapter, 0x70);
+		if (IS_ERR(ir->tx_c)) {
 			dev_err(&client->dev, "failed to setup tx i2c address");
+			err = PTR_ERR(ir->tx_c);
+			goto err_out_free;
 		} else if (!zilog_init(ir)) {
 			ir->carrier = 38000;
 			ir->duty_cycle = 40;
@@ -904,9 +907,6 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	return 0;
 
  err_out_free:
-	if (ir->tx_c)
-		i2c_unregister_device(ir->tx_c);
-
 	/* Only frees rc if it were allocated internally */
 	rc_free_device(rc);
 	return err;
@@ -919,9 +919,6 @@ static int ir_remove(struct i2c_client *client)
 	/* kill outstanding polls */
 	cancel_delayed_work_sync(&ir->work);
 
-	if (ir->tx_c)
-		i2c_unregister_device(ir->tx_c);
-
 	/* unregister device */
 	rc_unregister_device(ir->rc);
 
-- 
2.20.1

Re: [PATCH 1/2] media: ir-kbd-i2c: prevent potential NULL pointer access

[Sean Young]

On Mon, Jul 22, 2019 at 07:26:31PM +0200, Wolfram Sang wrote:
> i2c_new_dummy() can fail returning a NULL pointer. The code does not
> bail out in this case and the returned pointer is blindly used.

I don't see how. The existing code tries to set up the tx part; if
i2c_new_dummy() return NULL then the rcdev is registered without tx,
and tx_c is never used.

> Convert
> to devm_i2c_new_dummy_device() which returns an ERR_PTR and also bail
> out when failing the validity check.

Possibly I was being overly cautious with not bailing out if tx can't
be registered; moving to devm is probably a good idea. However the
commit message is misleading, because the existing code has no
NULL pointer access.

Sean

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/media/i2c/ir-kbd-i2c.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
> index 876d7587a1da..f46717052efc 100644
> --- a/drivers/media/i2c/ir-kbd-i2c.c
> +++ b/drivers/media/i2c/ir-kbd-i2c.c
> @@ -885,9 +885,12 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	INIT_DELAYED_WORK(&ir->work, ir_work);
>  
>  	if (probe_tx) {
> -		ir->tx_c = i2c_new_dummy(client->adapter, 0x70);
> -		if (!ir->tx_c) {
> +		ir->tx_c = devm_i2c_new_dummy_device(&client->dev,
> +						     client->adapter, 0x70);
> +		if (IS_ERR(ir->tx_c)) {
>  			dev_err(&client->dev, "failed to setup tx i2c address");
> +			err = PTR_ERR(ir->tx_c);
> +			goto err_out_free;
>  		} else if (!zilog_init(ir)) {
>  			ir->carrier = 38000;
>  			ir->duty_cycle = 40;
> @@ -904,9 +907,6 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	return 0;
>  
>   err_out_free:
> -	if (ir->tx_c)
> -		i2c_unregister_device(ir->tx_c);
> -
>  	/* Only frees rc if it were allocated internally */
>  	rc_free_device(rc);
>  	return err;
> @@ -919,9 +919,6 @@ static int ir_remove(struct i2c_client *client)
>  	/* kill outstanding polls */
>  	cancel_delayed_work_sync(&ir->work);
>  
> -	if (ir->tx_c)
> -		i2c_unregister_device(ir->tx_c);
> -
>  	/* unregister device */
>  	rc_unregister_device(ir->rc);
>  
> -- 
> 2.20.1

Re: [PATCH 1/2] media: ir-kbd-i2c: prevent potential NULL pointer access

[Wolfram Sang]

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

Hi Sean,

thanks for the review!

On Thu, Jul 25, 2019 at 06:12:02AM +0100, Sean Young wrote:
> On Mon, Jul 22, 2019 at 07:26:31PM +0200, Wolfram Sang wrote:
> > i2c_new_dummy() can fail returning a NULL pointer. The code does not
> > bail out in this case and the returned pointer is blindly used.
> 
> I don't see how. The existing code tries to set up the tx part; if
> i2c_new_dummy() return NULL then the rcdev is registered without tx,
> and tx_c is never used.

Yes, you are totally right. I missed that the send_block function is
also only called iff zilog_init succeeded. Thanks for the heads up and
sorry for the noise.

> 
> > Convert
> > to devm_i2c_new_dummy_device() which returns an ERR_PTR and also bail
> > out when failing the validity check.
> 
> Possibly I was being overly cautious with not bailing out if tx can't
> be registered; moving to devm is probably a good idea. However the
> commit message is misleading, because the existing code has no
> NULL pointer access.

Yep, I will resend with a proper commit message. Technically, there is
no need to bail out anymore because there is no NULL pointer access. My
tendency is now to not bail out and keep the old behaviour (registering
without tx). What do you think?

Regards,

   Wolfram


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

Re: [PATCH 1/2] media: ir-kbd-i2c: prevent potential NULL pointer access

[Sean Young]

Hi Wolfram,

On Thu, Jul 25, 2019 at 09:55:38AM +0200, Wolfram Sang wrote:
> Hi Sean,
> 
> thanks for the review!
> 
> On Thu, Jul 25, 2019 at 06:12:02AM +0100, Sean Young wrote:
> > On Mon, Jul 22, 2019 at 07:26:31PM +0200, Wolfram Sang wrote:
> > > i2c_new_dummy() can fail returning a NULL pointer. The code does not
> > > bail out in this case and the returned pointer is blindly used.
> > 
> > I don't see how. The existing code tries to set up the tx part; if
> > i2c_new_dummy() return NULL then the rcdev is registered without tx,
> > and tx_c is never used.
> 
> Yes, you are totally right. I missed that the send_block function is
> also only called iff zilog_init succeeded. Thanks for the heads up and
> sorry for the noise.

Not at all, thank you for the patch.

> > > Convert
> > > to devm_i2c_new_dummy_device() which returns an ERR_PTR and also bail
> > > out when failing the validity check.
> > 
> > Possibly I was being overly cautious with not bailing out if tx can't
> > be registered; moving to devm is probably a good idea. However the
> > commit message is misleading, because the existing code has no
> > NULL pointer access.
> 
> Yep, I will resend with a proper commit message. Technically, there is
> no need to bail out anymore because there is no NULL pointer access. My
> tendency is now to not bail out and keep the old behaviour (registering
> without tx). What do you think?

Since I write this code I've got pretty much every model with this zilog
transmitter/receiver, and they all work fine, including different firmware
versions. If there is a problem it would be nice to hear about it, and
not silently swallow the error. I think.

Thanks,

Sean

[PATCH 2/2] media: ir-kbd-i2c: remove outdated comments

[Wolfram Sang]

The "free memory" comment is obsolete since 2013 and the other ones
explain the obvious. Just remove the comments.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/media/i2c/ir-kbd-i2c.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
index f46717052efc..30fde0e025c9 100644
--- a/drivers/media/i2c/ir-kbd-i2c.c
+++ b/drivers/media/i2c/ir-kbd-i2c.c
@@ -916,13 +916,9 @@ static int ir_remove(struct i2c_client *client)
 {
 	struct IR_i2c *ir = i2c_get_clientdata(client);
 
-	/* kill outstanding polls */
 	cancel_delayed_work_sync(&ir->work);
-
-	/* unregister device */
 	rc_unregister_device(ir->rc);
 
-	/* free memory */
 	return 0;
 }
 
-- 
2.20.1