| 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
| ||
|
Date: Thu, 13 Apr 2023 13:06:50 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Baisong Zhong <zhongbaisong@...wei.com>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
mchehab@...nel.org, Adams.xu@...ave.com.cn,
Harshit Mogalapalli <harshit.m.mogalapalli@...il.com>
Subject: Re: [PATCH -next] media: dvb-usb: az6027: fix null-ptr-deref in
az6027_i2c_xfer()
On Sun, Nov 20, 2022 at 02:59:18PM +0800, Baisong Zhong wrote:
> Wei Chen reports a kernel bug as blew:
>
> general protection fault, probably for non-canonical address
> KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
> ...
> Call Trace:
> <TASK>
> __i2c_transfer+0x77e/0x1930 drivers/i2c/i2c-core-base.c:2109
> i2c_transfer+0x1d5/0x3d0 drivers/i2c/i2c-core-base.c:2170
> i2cdev_ioctl_rdwr+0x393/0x660 drivers/i2c/i2c-dev.c:297
> i2cdev_ioctl+0x75d/0x9f0 drivers/i2c/i2c-dev.c:458
> vfs_ioctl fs/ioctl.c:51 [inline]
> __do_sys_ioctl fs/ioctl.c:870 [inline]
> __se_sys_ioctl+0xfb/0x170 fs/ioctl.c:856
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x3d/0x90 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> RIP: 0033:0x7fd834a8bded
>
> In az6027_i2c_xfer(), if msg[i].addr is 0x99,
> a null-ptr-deref will caused when accessing msg[i].buf.
> For msg[i].len is 0 and msg[i].buf is null.
>
> Fix this by checking msg[i].len in az6027_i2c_xfer().
>
> Fixes: 76f9a820c867 ("V4L/DVB: AZ6027: Initial import of the driver")
> Link: https://lore.kernel.org/lkml/CAO4mrfcPHB5aQJO=mpqV+p8mPLNg-Fok0gw8gZ=zemAfMGTzMg@mail.gmail.com/
> Reported-by: Wei Chen <harperchen1110@...il.com>
> Signed-off-by: Baisong Zhong <zhongbaisong@...wei.com>
> ---
> drivers/media/usb/dvb-usb/az6027.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/usb/dvb-usb/az6027.c b/drivers/media/usb/dvb-usb/az6027.c
> index cf15988dfb51..7d78ee09be5e 100644
> --- a/drivers/media/usb/dvb-usb/az6027.c
> +++ b/drivers/media/usb/dvb-usb/az6027.c
> @@ -975,6 +975,10 @@ static int az6027_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], int n
> if (msg[i].addr == 0x99) {
> req = 0xBE;
> index = 0;
> + if (msg[i].len < 1) {
> + i = -EOPNOTSUPP;
> + break;
> + }
> value = msg[i].buf[0] & 0x00ff;
This is CVE-2023-28328 now. Why aren't the other msg[i].buf[0] accesses
checked? Is there a rule we could create so this could be detected by
Smatch?
I created a first draft Smatch warning that says whenever we access
msg[i].buf[] then we have to verify that msg[i].len was checked.
Attached.
CHECK drivers/media/usb/dvb-usb/az6027.c
drivers/media/usb/dvb-usb/az6027.c:991 az6027_i2c_xfer() warn: i2c_msg ->buf not checked 'msg[i]->len'
drivers/media/usb/dvb-usb/az6027.c:991 az6027_i2c_xfer() warn: i2c_msg ->buf not checked 'msg[i]->len'
drivers/media/usb/dvb-usb/az6027.c:1004 az6027_i2c_xfer() warn: i2c_msg ->buf not checked 'msg[i]->len'
drivers/media/usb/dvb-usb/az6027.c:1004 az6027_i2c_xfer() warn: i2c_msg ->buf not checked 'msg[i]->len'
drivers/media/usb/dvb-usb/az6027.c:1009 az6027_i2c_xfer() warn: i2c_msg ->buf not checked 'msg[i]->len'
drivers/media/usb/dvb-usb/az6027.c:1029 az6027_i2c_xfer() warn: i2c_msg ->buf not checked 'msg[i]->len'
It's a bug in Smatch that it's printing "msg[i]->len" instead of
"msg[i].len". :(
regards,
dan carpenter
drivers/media/usb/dvb-usb/az6027.c
973 for (i = 0; i < num; i++) {
974
975 if (msg[i].addr == 0x99) {
976 req = 0xBE;
977 index = 0;
978 if (msg[i].len < 1) {
^^^^^^^^^^^^^^
The new check is here.
979 i = -EOPNOTSUPP;
980 break;
981 }
982 value = msg[i].buf[0] & 0x00ff;
983 length = 1;
984 az6027_usb_out_op(d, req, value, index, data, length);
985 }
986
987 if (msg[i].addr == 0xd0) {
988 /* write/read request */
989 if (i + 1 < num && (msg[i + 1].flags & I2C_M_RD)) {
990 req = 0xB9;
991 index = (((msg[i].buf[0] << 8) & 0xff00) | (msg[i].buf[1] & 0x00ff));
^^^^^^^^^^^^^ ^^^^^^^^^^^^^
Two unchecked here.
992 value = msg[i].addr + (msg[i].len << 8);
993 length = msg[i + 1].len + 6;
994 az6027_usb_in_op(d, req, value, index, data, length);
995 len = msg[i + 1].len;
996 for (j = 0; j < len; j++)
997 msg[i + 1].buf[j] = data[j + 5];
998
999 i++;
1000 } else {
1001
1002 /* demod 16bit addr */
1003 req = 0xBD;
1004 index = (((msg[i].buf[0] << 8) & 0xff00) | (msg[i].buf[1] & 0x00ff));
^^^^^^^^^^^^^ ^^^^^^^^^^^^^
And here.
1005 value = msg[i].addr + (2 << 8);
1006 length = msg[i].len - 2;
1007 len = msg[i].len - 2;
1008 for (j = 0; j < len; j++)
1009 data[j] = msg[i].buf[j + 2];
This is a false positive because Smatch is not smart enough to track
that "len = msg[i].len - 2;" and "j < len;" means that "j + 2" is less
than < msg[i].len.
1010 az6027_usb_out_op(d, req, value, index, data, length);
1011 }
1012 }
1013
1014 if (msg[i].addr == 0xc0) {
1015 if (msg[i].flags & I2C_M_RD) {
1016
1017 req = 0xB9;
1018 index = 0x0;
1019 value = msg[i].addr;
1020 length = msg[i].len + 6;
1021 az6027_usb_in_op(d, req, value, index, data, length);
1022 len = msg[i].len;
1023 for (j = 0; j < len; j++)
1024 msg[i].buf[j] = data[j + 5];
1025
1026 } else {
1027
1028 req = 0xBD;
1029 index = msg[i].buf[0] & 0x00FF;
^^^^^^^^^^^^^
And here.
1030 value = msg[i].addr + (1 << 8);
1031 length = msg[i].len - 1;
1032 len = msg[i].len - 1;
1033
1034 for (j = 0; j < len; j++)
1035 data[j] = msg[i].buf[j + 1];
Apparently Smatch can figure this one out... Weird. I wonder if past
me made + 1 a special case...
1036
1037 az6027_usb_out_op(d, req, value, index, data, length);
1038 }
1039 }
1040 }
1041 mutex_unlock(&d->i2c_mutex);
1042 kfree(data);
1043
1044 return i;
1045 }
regards,
dan carpenter
View attachment "check_i2c_msg_buf.c" of type "text/x-csrc" (1930 bytes)
Powered by blists - more mailing lists