[v2 1/1] i2c: dev: prevent ZERO_SIZE_PTR deref in i2cdev_ioctl_rdwr()

[v2 1/1] i2c: dev: prevent ZERO_SIZE_PTR deref in i2cdev_ioctl_rdwr()

[Alexander Popov]

i2cdev_ioctl_rdwr() allocates i2c_msg.buf using memdup_user(), which
returns ZERO_SIZE_PTR if i2c_msg.len is zero.

Currently i2cdev_ioctl_rdwr() always dereferences the buf pointer in case
of I2C_M_RD | I2C_M_RECV_LEN transfer. That causes a kernel oops in
case of zero len.

Let's check the len against zero before dereferencing buf pointer.

This issue was triggered by syzkaller.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 drivers/i2c/i2c-dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 036a03f..5790bc8 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -280,6 +280,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 		 */
 		if (msgs[i].flags & I2C_M_RECV_LEN) {
 			if (!(msgs[i].flags & I2C_M_RD) ||
+			    !msgs[i].len ||
 			    msgs[i].buf[0] < 1 ||
 			    msgs[i].len < msgs[i].buf[0] +
 					     I2C_SMBUS_BLOCK_MAX) {
-- 
2.7.4

Re: [v2 1/1] i2c: dev: prevent ZERO_SIZE_PTR deref in i2cdev_ioctl_rdwr()

[Uwe Kleine-König]

On Thu, Apr 19, 2018 at 03:29:22PM +0300, Alexander Popov wrote:
> i2cdev_ioctl_rdwr() allocates i2c_msg.buf using memdup_user(), which
> returns ZERO_SIZE_PTR if i2c_msg.len is zero.
> 
> Currently i2cdev_ioctl_rdwr() always dereferences the buf pointer in case
> of I2C_M_RD | I2C_M_RECV_LEN transfer. That causes a kernel oops in
> case of zero len.
> 
> Let's check the len against zero before dereferencing buf pointer.
> 
> This issue was triggered by syzkaller.
> 
> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> ---
>  drivers/i2c/i2c-dev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 036a03f..5790bc8 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -280,6 +280,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  		 */
>  		if (msgs[i].flags & I2C_M_RECV_LEN) {
>  			if (!(msgs[i].flags & I2C_M_RD) ||
> +			    !msgs[i].len ||

I'd prefer

			msgs[i].len > 0

here instead of

			!msgs[i].len

because I can parse that more easily. Semantically the patch is fine and
can have my

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

>  			    msgs[i].buf[0] < 1 ||
>  			    msgs[i].len < msgs[i].buf[0] +
>  					     I2C_SMBUS_BLOCK_MAX) {

Best regards
Uwe

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

Re: [v2 1/1] i2c: dev: prevent ZERO_SIZE_PTR deref in i2cdev_ioctl_rdwr()

[Alexander Popov]

On 19.04.2018 16:49, Uwe Kleine-König wrote:
> On Thu, Apr 19, 2018 at 03:29:22PM +0300, Alexander Popov wrote:
>> i2cdev_ioctl_rdwr() allocates i2c_msg.buf using memdup_user(), which
>> returns ZERO_SIZE_PTR if i2c_msg.len is zero.
>>
>> Currently i2cdev_ioctl_rdwr() always dereferences the buf pointer in case
>> of I2C_M_RD | I2C_M_RECV_LEN transfer. That causes a kernel oops in
>> case of zero len.
>>
>> Let's check the len against zero before dereferencing buf pointer.
>>
>> This issue was triggered by syzkaller.
>>
>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
>> ---
>>  drivers/i2c/i2c-dev.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
>> index 036a03f..5790bc8 100644
>> --- a/drivers/i2c/i2c-dev.c
>> +++ b/drivers/i2c/i2c-dev.c
>> @@ -280,6 +280,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>>  		 */
>>  		if (msgs[i].flags & I2C_M_RECV_LEN) {
>>  			if (!(msgs[i].flags & I2C_M_RD) ||
>> +			    !msgs[i].len ||
> 
> I'd prefer
> 
> 			msgs[i].len > 0

Excuse me, it will be wrong. We stop if len is 0 to avoid the following
ZERO_SIZE_PTR dereference.

> here instead of
> 
> 			!msgs[i].len

I can change it to "msgs[i].len == 0". But is it really important?

I've carefully tested the current version with the original repro. It works correct.

> because I can parse that more easily. Semantically the patch is fine and
> can have my
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
>>  			    msgs[i].buf[0] < 1 ||
>>  			    msgs[i].len < msgs[i].buf[0] +
>>  					     I2C_SMBUS_BLOCK_MAX) {

Best regards,
Alexander

Re: [v2 1/1] i2c: dev: prevent ZERO_SIZE_PTR deref in i2cdev_ioctl_rdwr()

[Uwe Kleine-König]

Hello,

On Thu, Apr 19, 2018 at 08:01:46PM +0300, Alexander Popov wrote:
> On 19.04.2018 16:49, Uwe Kleine-König wrote:
> >> @@ -280,6 +280,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
> >>  		 */
> >>  		if (msgs[i].flags & I2C_M_RECV_LEN) {
> >>  			if (!(msgs[i].flags & I2C_M_RD) ||
> >> +			    !msgs[i].len ||
> > 
> > I'd prefer
> > 
> > 			msgs[i].len > 0
> 
> Excuse me, it will be wrong. We stop if len is 0 to avoid the following
> ZERO_SIZE_PTR dereference.

right you are. I missed the negation.
 
> > here instead of
> > 
> > 			!msgs[i].len
> 
> I can change it to "msgs[i].len == 0". But is it really important?
> 
> I've carefully tested the current version with the original repro. It works correct.

I don't doubt it, and the code generated is maybe even the same. The
point I wanted to make is that

	!len

is harder to read for a human than

	len < 1

(or another suitable arithmetic expression). But feel free to disagree
and keep the code as is.

Best regards
Uwe

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

Re: [v2 1/1] i2c: dev: prevent ZERO_SIZE_PTR deref in i2cdev_ioctl_rdwr()

[Alexander Popov]

On 19.04.2018 16:49, Uwe Kleine-König wrote:
> On Thu, Apr 19, 2018 at 03:29:22PM +0300, Alexander Popov wrote:
>> i2cdev_ioctl_rdwr() allocates i2c_msg.buf using memdup_user(), which
>> returns ZERO_SIZE_PTR if i2c_msg.len is zero.
>>
>> Currently i2cdev_ioctl_rdwr() always dereferences the buf pointer in case
>> of I2C_M_RD | I2C_M_RECV_LEN transfer. That causes a kernel oops in
>> case of zero len.
>>
>> Let's check the len against zero before dereferencing buf pointer.
>>
>> This issue was triggered by syzkaller.
>>
>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
>> ---
>>  drivers/i2c/i2c-dev.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
>> index 036a03f..5790bc8 100644
>> --- a/drivers/i2c/i2c-dev.c
>> +++ b/drivers/i2c/i2c-dev.c
>> @@ -280,6 +280,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>>  		 */
>>  		if (msgs[i].flags & I2C_M_RECV_LEN) {
>>  			if (!(msgs[i].flags & I2C_M_RD) ||
>> +			    !msgs[i].len ||
> 
> I'd prefer
> 
> 			msgs[i].len > 0
> 
> here instead of
> 
> 			!msgs[i].len
> 
> because I can parse that more easily. Semantically the patch is fine and
> can have my
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Friendly ping!

Wolfram, would you like to take this patch?

Best regards,
Alexander

Re: [v2 1/1] i2c: dev: prevent ZERO_SIZE_PTR deref in i2cdev_ioctl_rdwr()

[Wolfram Sang]

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

On Thu, Apr 19, 2018 at 03:29:22PM +0300, Alexander Popov wrote:
> i2cdev_ioctl_rdwr() allocates i2c_msg.buf using memdup_user(), which
> returns ZERO_SIZE_PTR if i2c_msg.len is zero.
> 
> Currently i2cdev_ioctl_rdwr() always dereferences the buf pointer in case
> of I2C_M_RD | I2C_M_RECV_LEN transfer. That causes a kernel oops in
> case of zero len.
> 
> Let's check the len against zero before dereferencing buf pointer.
> 
> This issue was triggered by syzkaller.
> 
> Signed-off-by: Alexander Popov <alex.popov@linux.com>

Applied to for-current with the arithmetic expression changed to '< 1'
to keep in sync with the previous one. Will push out soon, so you can
double check if you are interested.

Thanks for the debugging, Alexander!


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

Re: [v2 1/1] i2c: dev: prevent ZERO_SIZE_PTR deref in i2cdev_ioctl_rdwr()

[Uwe Kleine-König]

On Fri, Apr 27, 2018 at 02:06:58PM +0200, Wolfram Sang wrote:
> On Thu, Apr 19, 2018 at 03:29:22PM +0300, Alexander Popov wrote:
> > i2cdev_ioctl_rdwr() allocates i2c_msg.buf using memdup_user(), which
> > returns ZERO_SIZE_PTR if i2c_msg.len is zero.
> > 
> > Currently i2cdev_ioctl_rdwr() always dereferences the buf pointer in case
> > of I2C_M_RD | I2C_M_RECV_LEN transfer. That causes a kernel oops in
> > case of zero len.
> > 
> > Let's check the len against zero before dereferencing buf pointer.
> > 
> > This issue was triggered by syzkaller.
> > 
> > Signed-off-by: Alexander Popov <alex.popov@linux.com>
> 
> Applied to for-current with the arithmetic expression changed to '< 1'
> to keep in sync with the previous one. Will push out soon, so you can
> double check if you are interested.

Thanks, I like it. An added bonus is also that you don't need to think
about the type of msgs[i].len and what happens if it is negative.

Best regards
Uwe


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