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@xxxxxxxxxxxxxx/
> Reported-by: Wei Chen <harperchen1110@xxxxxxxxx>
> Signed-off-by: Baisong Zhong <zhongbaisong@xxxxxxxxxx>
> ---
> 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
/*
* Copyright (C) 2023 Dan Carpenter.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
*/
#include "smatch.h"
#include "smatch_slist.h"
#include "smatch_extra.h"
static int my_id;
static struct expression *get_len_expr(struct expression *expr)
{
struct expression *deref;
if (expr->type != EXPR_DEREF)
return NULL;
deref = strip_expr(expr->deref);
if (local_debug)
sm_msg("%s: expr='%s' op='%c'", __func__, expr_to_str(expr), expr->op);
return gen_expression_from_key(deref, (expr->op == '.') ? "$.len" : "$->len");
}
static void array_check(struct expression *expr)
{
struct expression *array_expr, *offset, *len;
char *member, *name;
int comparison;
expr = strip_expr(expr);
array_expr = get_array_base(expr);
offset = get_array_offset(expr);
if (!array_expr || !offset)
return;
member = get_member_name(array_expr);
if (!member || strcmp(member, "(struct i2c_msg)->buf") != 0)
goto free;
len = get_len_expr(array_expr);
if (!len)
goto free;
comparison = get_comparison(offset, len);
if (comparison == '<')
goto free;
name = expr_to_str(len);
sm_warning("i2c_msg ->buf not checked '%s'", name);
free_string(name);
free:
free_string(member);
}
void check_i2c_msg_buf(int id)
{
my_id = id;
if (option_project != PROJ_KERNEL)
return;
add_hook(&array_check, OP_HOOK);
}
