[PATCH 1/5] accel/qaic: tighten bounds checking in encode_message()

[Pranjal Ramajor Asha Kanojiya]

On 7/4/2023 2:08 PM, Dan Carpenter wrote:
> On Tue, Jul 04, 2023 at 11:57:51AM +0530, Pranjal Ramajor Asha Kanojiya wrote:
>>> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
>>> index 5c57f7b4494e..a51b1594dcfa 100644
>>> --- a/drivers/accel/qaic/qaic_control.c
>>> +++ b/drivers/accel/qaic/qaic_control.c
>>> @@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
>>>    	int ret;
>>>    	int i;
>>> -	if (!user_msg->count) {
>>> +	if (!user_msg->count ||
>>> +	    user_msg->len < sizeof(*trans_hdr)) {
>>>    		ret = -EINVAL;
>>>    		goto out;
>>>    	}
>>> @@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
>>>    	}
>>>    	for (i = 0; i < user_msg->count; ++i) {
>>> -		if (user_len >= user_msg->len) {
>>> +		if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
>> If I understand correctly this check is added to verify if we are left with
>> trans_hdr size of data. In that case '>' comparison operator should be used.
> 
> That was there in the original code and I thought about changing it but
> I don't like changing things which aren't necessary and == is also
> invalid so I decided to leave it.
> 
I see, I understand your concern about not changing unnecessary things 
but '>=' is incorrect for reason mentioned above. We need to change that 
to '>'
>>
>>>    			ret = -EINVAL;
>>>    			break;
>>>    		}
>>>    		trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data + user_len);
>>> -		if (user_len + trans_hdr->len > user_msg->len) {
>>> +		if (trans_hdr->len < sizeof(trans_hdr) ||
>>> +		    size_add(user_len, trans_hdr->len) > user_msg->len) {
> 
> If we change to > then the == will be caught by this check.  So it
> doesn't affect runtime either way.
> 
I fail to see that.

Lets run an example:
user_len is 0
user_msg->len is 8
sizeof(*trans_hdr) is 8
trans_hdr->len is 8

Above instance is correct and should be processed without error.

So user_len > user_msg->len - sizeof(*trans_hdr) translates to
(0 > 8 - 8)
(0 > 0)
false (No error)
.
.
.
trans_hdr->len < sizeof(trans_hdr) ||
size_add(user_len, trans_hdr->len) > user_msg->len, translates to
8 < 8 || size_add(0, 8) > 8
false || 8 > 8
false || false
false (No error)

Am I missing anything?

> regards,
> dan carpenter
>