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

Message ID 20190722172632.4402-2-wsa+renesas@sang-engineering.com
State Superseded
Headers show
Series
  • media: ir-kbd-i2c: fix potential OOPS & minor cleanup
Related show

Commit Message

Wolfram Sang July 22, 2019, 5:26 p.m. UTC
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(-)

Comments

Sean Young July 25, 2019, 5:12 a.m. UTC | #1
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
Wolfram Sang July 25, 2019, 7:55 a.m. UTC | #2
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
Sean Young July 25, 2019, 10:44 a.m. UTC | #3
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
diff mbox series

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);