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

[Jeffrey Hugo]

On 7/4/2023 3:58 AM, Dan Carpenter wrote:
> On Tue, Jul 04, 2023 at 03:18:26PM +0530, Pranjal Ramajor Asha Kanojiya wrote:
>>
>>
>> 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 '>'
> 
> Oh, yes.  You're right.  I will need to resend.

For the next revision, please add #include <overflow.h>
I believe the size_add() use that you propose is the first need of that 
file, and while it may be implicitly included from something we do 
include, I prefer to have explicit includes.

Otherwise I don't see anything else to add.