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
