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@gmail.com>
> Signed-off-by: Baisong Zhong <zhongbaisong@huawei.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