linux-kernel - Re: [PATCH 1/1] i2c: dev: check i2c_msg len before memdup_user() to prevent ZERO_SIZE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 18 Apr 2018 16:00:23 +0300
From:   Alexander Popov <alex.popov@...ux.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Wolfram Sang <wsa@...-dreams.de>, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org, sil2review@...ts.osadl.org,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [PATCH 1/1] i2c: dev: check i2c_msg len before memdup_user() to
 prevent ZERO_SIZE_PTR deref

On 18.04.2018 11:23, Uwe Kleine-König wrote:
> On Wed, Apr 18, 2018 at 10:56:03AM +0300, Alexander Popov wrote:
>> On 18.04.2018 10:07, Uwe Kleine-König wrote:
>>> Your commit log is wrong (and I think the patch, too).
>>
>> I believe this bug is not a memdup_user() issue. There is a nice selection from
>> LKML discussions about ZERO_SIZE_PTR, which convinces me:
>> http://yarchive.net/comp/linux/malloc_0.html
> 
> Ack, no memdup_user problem. i2cdev_ioctl_rdwr() should not access
> msgs[i].buf[0] if msgs[i].len is 0.
> 
> But you should not prohibit i2c transfers with length 0 in general.

Ok, thanks for that info. I should fix the patch.

> So a better patch is the following:
> 
> 
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 036a03f0d0a6..0137538c36a0 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 < 1 ||
>  			    msgs[i].buf[0] < 1 ||
>  			    msgs[i].len < msgs[i].buf[0] +
>  					     I2C_SMBUS_BLOCK_MAX) {
> 
> But having said that and after reading the comment above the if, I'm not
> sure this is enough. 

I'll check that carefully and come back with the next version.

Thanks!
Alexander