[0/5] accel/qaic: Improve bounds checking in encode/decode

Message

Dan Carpenter June 21, 2023, 7:21 a.m. UTC

(I think this is the first cover letter I have ever written).

These patches are based on review and not from testing.

I found it quite complicated to track the buffer sizes.  What happens
is the qaic_manage() gets a buffer user_msg->data[] which has
user_msg->len bytes.  The qaic_manage() calls qaic_manage_msg_xfer()
which encodes the user's message.

Then we get a response and we decode the response back into
user_msg->data[], but we don't check that it is overflowed.  We instead
copy seem to check against msg_hdr_len (which would prevent a read
overflow).  At the end user_msg->len gets set to the number of bytes
that we copied to the buffer.

I'm coming to this code brand new, it's the first time I have seen it.
So I don't really understand.  There is an element of trust in
msg_hdr_len but then at other times we check it for integer overflows
which indicates deep distrust.

What I'm saying is that there may be more issues in this code.  But also
that I don't really understand it so please review carefully.

The patch that I'm least sure of is 4/5:

[PATCH 4/5] accel/qaic: move and expand integer overflow checks for
 map_user_pages()

regards,
dan carpenter

Comments

Jeffrey Hugo June 22, 2023, 2:53 a.m. UTC | #1

On 6/21/2023 1:21 AM, Dan Carpenter wrote:
> (I think this is the first cover letter I have ever written).
> 
> These patches are based on review and not from testing.

Thank you for your review.  I look forward to reading your patches and 
learning from them.

Did you use any kind of tooling?  If there is something we can add to 
our flow to bring up the quality, I would like to consider it.

Tooling or no, the control path is not a trivial part of the driver to 
dip your toes in, and it seems like you really dug deep.  I find that 
impressive.

> I found it quite complicated to track the buffer sizes.  What happens
> is the qaic_manage() gets a buffer user_msg->data[] which has
> user_msg->len bytes.  The qaic_manage() calls qaic_manage_msg_xfer()
> which encodes the user's message.
> 
> Then we get a response and we decode the response back into
> user_msg->data[], but we don't check that it is overflowed.  We instead
> copy seem to check against msg_hdr_len (which would prevent a read
> overflow).  At the end user_msg->len gets set to the number of bytes
> that we copied to the buffer.
> 
> I'm coming to this code brand new, it's the first time I have seen it.
> So I don't really understand.  There is an element of trust in
> msg_hdr_len but then at other times we check it for integer overflows
> which indicates deep distrust.

Overall, we are taking a message from userspace and transforming it into 
something the firmware on the device can consume.  Then we get a 
response back from the firmware, and transform that back into something 
userspace can consume.  From the driver perspective, neither the 
firmware nor userspace is really trusted.  msg_hdr_len is something that 
the driver calculates and maintains, but is updated with untrusted values.

I can see where that could be confusing.  I look forward to looking at 
what you've found, and hopefully making the code better.

> What I'm saying is that there may be more issues in this code.  But also
> that I don't really understand it so please review carefully.
> 
> The patch that I'm least sure of is 4/5:
> 
> [PATCH 4/5] accel/qaic: move and expand integer overflow checks for
>   map_user_pages()
> 
> regards,
> dan carpenter

Dan Carpenter June 22, 2023, 10:22 a.m. UTC | #2

On Wed, Jun 21, 2023 at 08:53:41PM -0600, Jeffrey Hugo wrote:
> On 6/21/2023 1:21 AM, Dan Carpenter wrote:
> > (I think this is the first cover letter I have ever written).
> > 
> > These patches are based on review and not from testing.
> 
> Thank you for your review.  I look forward to reading your patches and
> learning from them.
> 
> Did you use any kind of tooling?  If there is something we can add to our
> flow to bring up the quality, I would like to consider it.

I started reviewing this code because of an unpublished Smatch warning:

drivers/accel/qaic/qaic_control.c:379 encode_passthrough() warn: check that subtract can't underflow 'in_trans->hdr.len - 8' '0-3999968'

The warning message means that Smatch thinks in_trans->hdr.len can be
controlled by the user and is in the 0-3999968.  But from review it's
in increments of 8.  "0,8,16...3999968".

The other subtract underflow warnings are false positives except maybe
cx231xx_bulk_copy()?  The put_cmsg() and the bpf warnings are definitely
false positives.

drivers/accel/qaic/qaic_control.c:379 encode_passthrough() warn: check that subtract can't underflow 'in_trans->hdr.len - 8' '0-3999968'
drivers/media/usb/cx231xx/cx231xx-417.c:1355 cx231xx_bulk_copy() warn: check that subtract can't underflow 'buffer_size - 3' '0-4000000'
drivers/net/ethernet/microchip/sparx5/sparx5_packet.c:153 sparx5_xtr_grp() warn: check that subtract can't underflow 'byte_cnt - 4' '0'
drivers/net/ethernet/packetengines/hamachi.c:1504 hamachi_rx() warn: check that subtract can't underflow '(frame_status & 2047) - 4' '0-2047'
drivers/net/ethernet/packetengines/hamachi.c:1506 hamachi_rx() warn: check that subtract can't underflow '(frame_status & 2047) - 4' '0-2047'
drivers/net/ethernet/packetengines/hamachi.c:1520 hamachi_rx() warn: check that subtract can't underflow '(frame_status & 2047) - 4' '0-2047'
fs/ubifs/debug.c:334 ubifs_dump_node() warn: check that subtract can't underflow 'safe_len - 24' 's32min-(-1),25-2147483646'
fs/ubifs/debug.c:512 ubifs_dump_node() warn: check that subtract can't underflow 'safe_len - 48' 's32min-s32max'
kernel/bpf/bpf_iter.c:479 bpf_iter_link_fill_link_info() warn: check that subtract can't underflow 'ulen - 1' '0-1010101'
kernel/bpf/btf.c:7274 btf_get_info_by_fd() warn: check that subtract can't underflow 'uname_len - 1' '0-55'
kernel/bpf/syscall.c:3268 bpf_raw_tp_link_fill_link_info() warn: check that subtract can't underflow 'ulen - 1' '0-1010101'
net/compat.c:273 put_cmsg_compat() warn: check that subtract can't underflow 'cmlen - 12' 's32min-s32max'
net/core/scm.c:249 put_cmsg() warn: check that subtract can't underflow 'cmlen - 16' 's32min-s32max'

regards,
dan carpenter