i2c: dev: prevent adapter retries being set as minus value

If set adapter->retries to minus value from user space via ioctl,
will make __i2c_transfer and __i2c_smbus_xfer jump the calling to
adapter->algo->master_xfer and adapter->algo->smbus_xfer that
registered by the underlying bus drivers, and return value 0 to
all the callers. The bus driver will never be accessed anymore by
all users, besides, the users may still get successful return value
with no any error or information log print out.

Signed-off-by: Yi Zeng <yizeng@asrmicro.com>
---
 drivers/i2c/i2c-dev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Hi Yi Zeng,

On Fri, Dec 21, 2018 at 05:32:06PM +0800, Yi Zeng wrote:
> If set adapter->retries to minus value from user space via ioctl,
> will make __i2c_transfer and __i2c_smbus_xfer jump the calling to
> adapter->algo->master_xfer and adapter->algo->smbus_xfer that
> registered by the underlying bus drivers, and return value 0 to
> all the callers. The bus driver will never be accessed anymore by
> all users, besides, the users may still get successful return value
> with no any error or information log print out.

Thanks! The issue you observed is correct. It also applies to
I2C_TIMEOUT. Would you mind fixing it there as well?

> Signed-off-by: Yi Zeng <yizeng@asrmicro.com>
> ---
>  drivers/i2c/i2c-dev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 1aca742..c349f58 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -470,6 +470,14 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  					  data_arg.data);
>  	}
>  	case I2C_RETRIES:
> +		/*
> +		 * The adapter->retries is defined as int type, and as
> +		 * the upper limit for times of i2c transfer retry when
> +		 * get -EAGAIN, it should not be set as minus value.
> +		 */

I usually like comments explaining the situiation. However, here I think
it is pretty clear that the code does just sanity checks. So, I think we
can drop it.

> +		if ((int)arg < 0)
> +			return -EINVAL;

Minor nit: I'd think this is a little more readable

	if (arg > INT_MAX)
			return -EINVAL

But I have no strong opinion here.

Kind regards,

   Wolfram

Hi Wolfram Sang,

Thank you very much for your review and kindly suggestions, would you please see my comments below:

Best Regards,
Yi Zeng
+86-21-60336588 ext. 8686


-----Original Message-----
From: Wolfram Sang [mailto:wsa@the-dreams.de] 
Sent: Friday, January 04, 2019 3:30 AM
To: Zeng Yi（曾毅）
Cc: linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i2c: dev: prevent adapter retries being set as minus value

Hi Yi Zeng,

On Fri, Dec 21, 2018 at 05:32:06PM +0800, Yi Zeng wrote:
> If set adapter->retries to minus value from user space via ioctl, will 
> make __i2c_transfer and __i2c_smbus_xfer jump the calling to
> adapter->algo->master_xfer and adapter->algo->smbus_xfer that
> registered by the underlying bus drivers, and return value 0 to all 
> the callers. The bus driver will never be accessed anymore by all 
> users, besides, the users may still get successful return value with 
> no any error or information log print out.

>> Thanks! The issue you observed is correct. It also applies to I2C_TIMEOUT. Would you mind fixing it there as well?
Yes, I am very glad to do this fix. I will add the changes to the previous patch.

> Signed-off-by: Yi Zeng <yizeng@asrmicro.com>
> ---
>  drivers/i2c/i2c-dev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c index 
> 1aca742..c349f58 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -470,6 +470,14 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  					  data_arg.data);
>  	}
>  	case I2C_RETRIES:
> +		/*
> +		 * The adapter->retries is defined as int type, and as
> +		 * the upper limit for times of i2c transfer retry when
> +		 * get -EAGAIN, it should not be set as minus value.
> +		 */

>> I usually like comments explaining the situiation. However, here I think it is pretty clear that the code does just sanity checks. So, I think we can drop it.
Thank you, I will drop it in the updates.

> +		if ((int)arg < 0)
> +			return -EINVAL;

>> Minor nit: I'd think this is a little more readable

>>	if (arg > INT_MAX)
>>			return -EINVAL

>> But I have no strong opinion here.
Thank you very much for your suggestion, I think this is much better than the previous.

Kind regards,

   Wolfram