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

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

[Dan Carpenter]

(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

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

[Dan Carpenter]

There are several issues in this code.  The check at the start of the
loop:

	if (user_len >= user_msg->len) {

This check does not ensure that we have enough space for the trans_hdr
(8 bytes).  Instead the check needs to be:

	if (user_len >= user_msg->len - sizeof(*trans_hdr)) {

That subtraction is done as an unsigned long we want to avoid
negatives.  Add a lower bound to the start of the function.

	if (user_msg->len < sizeof(*trans_hdr))

There is a second integer underflow which can happen if
trans_hdr->len is zero inside the encode_passthrough() function.

	memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - sizeof(in_trans->hdr));

Instead of adding a check to encode_passthrough() it's better to check
in this central place.  Add that check:

	if (trans_hdr->len < sizeof(trans_hdr)

The final concern is that the "user_len + trans_hdr->len" might have an
integer overflow bug.  Use size_add() to prevent that.

-	if (user_len + trans_hdr->len > user_msg->len) {
+	if (size_add(user_len, trans_hdr->len) > user_msg->len) {

Fixes: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
This is based on code review and not tested.

 drivers/accel/qaic/qaic_control.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

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)) {
 			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) {
 			ret = -EINVAL;
 			break;
 		}
-- 
2.39.2

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

[Pranjal Ramajor Asha Kanojiya]

On 6/21/2023 12:51 PM, Dan Carpenter wrote:
> There are several issues in this code.  The check at the start of the
> loop:
> 
> 	if (user_len >= user_msg->len) {
> 
> This check does not ensure that we have enough space for the trans_hdr
> (8 bytes).  Instead the check needs to be:
> 
> 	if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
> 
> That subtraction is done as an unsigned long we want to avoid
> negatives.  Add a lower bound to the start of the function.
> 
> 	if (user_msg->len < sizeof(*trans_hdr))
> 
> There is a second integer underflow which can happen if
> trans_hdr->len is zero inside the encode_passthrough() function.
> 
> 	memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - sizeof(in_trans->hdr));
> 
> Instead of adding a check to encode_passthrough() it's better to check
> in this central place.  Add that check:
> 
> 	if (trans_hdr->len < sizeof(trans_hdr)
> 
> The final concern is that the "user_len + trans_hdr->len" might have an
> integer overflow bug.  Use size_add() to prevent that.
> 
> -	if (user_len + trans_hdr->len > user_msg->len) {
> +	if (size_add(user_len, trans_hdr->len) > user_msg->len) {
> 
> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> This is based on code review and not tested.
> 
>   drivers/accel/qaic/qaic_control.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> 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)) {
Can we have something like this here
user_msg->len < sizeof(*trans_hdr) * user_msg->count, no?
>   		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)) {
Do you think it is more readable if we have something like this
user_len + sizeof(*trans_hdr) >= user_msg->len
>   			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) {
>   			ret = -EINVAL;
>   			break;
>   		}

Hey Dan, Thank you for going through qaic driver. You patches are very 
much appreciated. This is good work.

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

[Dan Carpenter]

On Thu, Jun 22, 2023 at 04:54:03PM +0530, Pranjal Ramajor Asha Kanojiya wrote:
> 
> 
> On 6/21/2023 12:51 PM, Dan Carpenter wrote:
> > There are several issues in this code.  The check at the start of the
> > loop:
> > 
> > 	if (user_len >= user_msg->len) {
> > 
> > This check does not ensure that we have enough space for the trans_hdr
> > (8 bytes).  Instead the check needs to be:
> > 
> > 	if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
> > 
> > That subtraction is done as an unsigned long we want to avoid
> > negatives.  Add a lower bound to the start of the function.
> > 
> > 	if (user_msg->len < sizeof(*trans_hdr))
> > 
> > There is a second integer underflow which can happen if
> > trans_hdr->len is zero inside the encode_passthrough() function.
> > 
> > 	memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - sizeof(in_trans->hdr));
> > 
> > Instead of adding a check to encode_passthrough() it's better to check
> > in this central place.  Add that check:
> > 
> > 	if (trans_hdr->len < sizeof(trans_hdr)
> > 
> > The final concern is that the "user_len + trans_hdr->len" might have an
> > integer overflow bug.  Use size_add() to prevent that.
> > 
> > -	if (user_len + trans_hdr->len > user_msg->len) {
> > +	if (size_add(user_len, trans_hdr->len) > user_msg->len) {
> > 
> > Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > This is based on code review and not tested.
> > 
> >   drivers/accel/qaic/qaic_control.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > 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)) {
> Can we have something like this here
> user_msg->len < sizeof(*trans_hdr) * user_msg->count, no?

This check was just to ensure that we have space for one header so that
the "user_msg->len - sizeof(*trans_hdr)" subtraction doesn't overflow.
We're going to need to check that we have space for each header later
anyway.  Can the multiply fail (on 32bit)?

> >   		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)) {
> Do you think it is more readable if we have something like this
> user_len + sizeof(*trans_hdr) >= user_msg->len

Either way works.  The math should be on trusted side, and to me the
form is always if (variable >= trusted value) { so I prefer to put the
math on right.  But here both sides are trusted and there is no risk of
integer overflow.  If we did that then we could remove the

	if (user_msg->len < sizeof(*trans_hdr))

condition from the start.

regards,
dan carpenter

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

[Dan Carpenter]

On Thu, Jun 22, 2023 at 02:43:57PM +0300, Dan Carpenter wrote:
> > > -	if (!user_msg->count) {
> > > +	if (!user_msg->count ||
> > > +	    user_msg->len < sizeof(*trans_hdr)) {
> > Can we have something like this here
> > user_msg->len < sizeof(*trans_hdr) * user_msg->count, no?
> 
> This check was just to ensure that we have space for one header so that
> the "user_msg->len - sizeof(*trans_hdr)" subtraction doesn't overflow.
> We're going to need to check that we have space for each header later
> anyway.  Can the multiply fail (on 32bit)?

s/fail/integer overflow/.  Obviously failure is not an option when it
comes to multiplies.

regards,
dan carpenter

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

[Pranjal Ramajor Asha Kanojiya]

On 6/21/2023 12:51 PM, Dan Carpenter wrote:
> There are several issues in this code.  The check at the start of the
> loop:
> 
> 	if (user_len >= user_msg->len) {
> 
> This check does not ensure that we have enough space for the trans_hdr
> (8 bytes).  Instead the check needs to be:
> 
> 	if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
> 
> That subtraction is done as an unsigned long we want to avoid
> negatives.  Add a lower bound to the start of the function.
> 
> 	if (user_msg->len < sizeof(*trans_hdr))
> 
> There is a second integer underflow which can happen if
> trans_hdr->len is zero inside the encode_passthrough() function.
> 
> 	memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - sizeof(in_trans->hdr));
> 
> Instead of adding a check to encode_passthrough() it's better to check
> in this central place.  Add that check:
> 
> 	if (trans_hdr->len < sizeof(trans_hdr)
> 
> The final concern is that the "user_len + trans_hdr->len" might have an
> integer overflow bug.  Use size_add() to prevent that.
> 
> -	if (user_len + trans_hdr->len > user_msg->len) {
> +	if (size_add(user_len, trans_hdr->len) > user_msg->len) {
> 
> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> This is based on code review and not tested.
> 
>   drivers/accel/qaic/qaic_control.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> 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.

>   			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) {
>   			ret = -EINVAL;
>   			break;
>   		}

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

[Pranjal Ramajor Asha Kanojiya]

On 7/4/2023 11:57 AM, Pranjal Ramajor Asha Kanojiya wrote:
> 
> 
> On 6/21/2023 12:51 PM, Dan Carpenter wrote:
>> There are several issues in this code.  The check at the start of the
>> loop:
>>
>>     if (user_len >= user_msg->len) {
>>
>> This check does not ensure that we have enough space for the trans_hdr
>> (8 bytes).  Instead the check needs to be:
>>
>>     if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
>>
>> That subtraction is done as an unsigned long we want to avoid
>> negatives.  Add a lower bound to the start of the function.
>>
>>     if (user_msg->len < sizeof(*trans_hdr))
>>
>> There is a second integer underflow which can happen if
>> trans_hdr->len is zero inside the encode_passthrough() function.
>>
>>     memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - 
>> sizeof(in_trans->hdr));
>>
>> Instead of adding a check to encode_passthrough() it's better to check
>> in this central place.  Add that check:
>>
>>     if (trans_hdr->len < sizeof(trans_hdr)
>>
>> The final concern is that the "user_len + trans_hdr->len" might have an
>> integer overflow bug.  Use size_add() to prevent that.
>>
>> -    if (user_len + trans_hdr->len > user_msg->len) {
>> +    if (size_add(user_len, trans_hdr->len) > user_msg->len) {
>>
>> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> ---
>> This is based on code review and not tested.
>>
>>   drivers/accel/qaic/qaic_control.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> 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.
> 
>>               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) {
Since the size of characters per line is 100 now. Can we rearrange this 
if condition and have them in one line. Similarity at other places in 
this patch series.

Thank you.
>>               ret = -EINVAL;
>>               break;
>>           }

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

[Dan Carpenter]

On Tue, Jul 04, 2023 at 12:04:01PM +0530, Pranjal Ramajor Asha Kanojiya wrote:
> > > -??????? 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) {
> Since the size of characters per line is 100 now. Can we rearrange this if
> condition and have them in one line. Similarity at other places in this
> patch series.

Style is subjective so I can't say for sure that my style is better but
obviously it is.  ;)  Those are two separate conditions so I put them on
two lines.  If it were something very related like if (x < 0 || x >= 10)
then I would have put it on one line.

regards,
dan carpenter

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

[Dan Carpenter]

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.

> 
> >   			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.

regards,
dan carpenter

Re: [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
> 

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

[Dan Carpenter]

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.

regards,
dan carpenter

Re: [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.

[PATCH 2/5] accel/qaic: tighten bounds checking in decode_message()

[Dan Carpenter]

Copy the bounds checking from encode_message() to decode_message().

This patch addresses the following concerns.  Ensure that there is
enough space for at least one header so that we don't have a negative
size later.

	if (msg_hdr_len < sizeof(*trans_hdr))

Ensure that we have enough space to read the next header from the
msg->data.

	if (msg_len >= msg_hdr_len - sizeof(*trans_hdr))
		return -EINVAL;

Check that the trans_hdr->len is not below the minimum size:

	if (hdr_len < sizeof(*trans_hdr))

This minimum check ensures that we don't corrupt memory in
decode_passthrough() when we do.

	memcpy(out_trans->data, in_trans->data, len - sizeof(in_trans->hdr));

And finally, use size_add() to prevent an integer overflow:

	if (size_add(msg_len, hdr_len) > msg_hdr_len)

Fixes: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/accel/qaic/qaic_control.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
index a51b1594dcfa..78f6c3d6380d 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -955,15 +955,23 @@ static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
 	int ret;
 	int i;
 
-	if (msg_hdr_len > QAIC_MANAGE_MAX_MSG_LENGTH)
+	if (msg_hdr_len < sizeof(*trans_hdr) ||
+	    msg_hdr_len > QAIC_MANAGE_MAX_MSG_LENGTH)
 		return -EINVAL;
 
 	user_msg->len = 0;
 	user_msg->count = le32_to_cpu(msg->hdr.count);
 
 	for (i = 0; i < user_msg->count; ++i) {
+		u32 hdr_len;
+
+		if (msg_len >= msg_hdr_len - sizeof(*trans_hdr))
+			return -EINVAL;
+
 		trans_hdr = (struct wire_trans_hdr *)(msg->data + msg_len);
-		if (msg_len + le32_to_cpu(trans_hdr->len) > msg_hdr_len)
+		hdr_len = le32_to_cpu(trans_hdr->len);
+		if (hdr_len < sizeof(*trans_hdr) ||
+		    size_add(msg_len, hdr_len) > msg_hdr_len)
 			return -EINVAL;
 
 		switch (le32_to_cpu(trans_hdr->type)) {
-- 
2.39.2

Re: [PATCH 2/5] accel/qaic: tighten bounds checking in decode_message()

[Pranjal Ramajor Asha Kanojiya]

On 6/21/2023 12:51 PM, Dan Carpenter wrote:
> Copy the bounds checking from encode_message() to decode_message().
> 
> This patch addresses the following concerns.  Ensure that there is
> enough space for at least one header so that we don't have a negative
> size later.
> 
> 	if (msg_hdr_len < sizeof(*trans_hdr))
> 
> Ensure that we have enough space to read the next header from the
> msg->data.
> 
> 	if (msg_len >= msg_hdr_len - sizeof(*trans_hdr))
> 		return -EINVAL;
> 
> Check that the trans_hdr->len is not below the minimum size:
> 
> 	if (hdr_len < sizeof(*trans_hdr))
> 
> This minimum check ensures that we don't corrupt memory in
> decode_passthrough() when we do.
> 
> 	memcpy(out_trans->data, in_trans->data, len - sizeof(in_trans->hdr));
> 
> And finally, use size_add() to prevent an integer overflow:
> 
> 	if (size_add(msg_len, hdr_len) > msg_hdr_len)
> 
> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>   drivers/accel/qaic/qaic_control.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
> index a51b1594dcfa..78f6c3d6380d 100644
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
> @@ -955,15 +955,23 @@ static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
>   	int ret;
>   	int i;
>   
> -	if (msg_hdr_len > QAIC_MANAGE_MAX_MSG_LENGTH)
> +	if (msg_hdr_len < sizeof(*trans_hdr) ||
> +	    msg_hdr_len > QAIC_MANAGE_MAX_MSG_LENGTH)
>   		return -EINVAL;
>   
>   	user_msg->len = 0;
>   	user_msg->count = le32_to_cpu(msg->hdr.count);
>   
>   	for (i = 0; i < user_msg->count; ++i) {
> +		u32 hdr_len;
> +
> +		if (msg_len >= msg_hdr_len - sizeof(*trans_hdr))
> +			return -EINVAL;
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.
> +
>   		trans_hdr = (struct wire_trans_hdr *)(msg->data + msg_len);
> -		if (msg_len + le32_to_cpu(trans_hdr->len) > msg_hdr_len)
> +		hdr_len = le32_to_cpu(trans_hdr->len);
> +		if (hdr_len < sizeof(*trans_hdr) ||
> +		    size_add(msg_len, hdr_len) > msg_hdr_len)
>   			return -EINVAL;
>   
>   		switch (le32_to_cpu(trans_hdr->type)) {

Re: [PATCH 2/5] accel/qaic: tighten bounds checking in decode_message()

[Jeffrey Hugo]

On 6/21/2023 1:21 AM, Dan Carpenter wrote:
> Copy the bounds checking from encode_message() to decode_message().
> 
> This patch addresses the following concerns.  Ensure that there is
> enough space for at least one header so that we don't have a negative
> size later.
> 
> 	if (msg_hdr_len < sizeof(*trans_hdr))
> 
> Ensure that we have enough space to read the next header from the
> msg->data.
> 
> 	if (msg_len >= msg_hdr_len - sizeof(*trans_hdr))
> 		return -EINVAL;
> 
> Check that the trans_hdr->len is not below the minimum size:
> 
> 	if (hdr_len < sizeof(*trans_hdr))
> 
> This minimum check ensures that we don't corrupt memory in
> decode_passthrough() when we do.
> 
> 	memcpy(out_trans->data, in_trans->data, len - sizeof(in_trans->hdr));
> 
> And finally, use size_add() to prevent an integer overflow:
> 
> 	if (size_add(msg_len, hdr_len) > msg_hdr_len)
> 
> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>   drivers/accel/qaic/qaic_control.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
> index a51b1594dcfa..78f6c3d6380d 100644
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
> @@ -955,15 +955,23 @@ static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
>   	int ret;
>   	int i;
>   
> -	if (msg_hdr_len > QAIC_MANAGE_MAX_MSG_LENGTH)
> +	if (msg_hdr_len < sizeof(*trans_hdr) ||

How I view this - does the specified message length contain at-least one 
transaction for us to decode?

Seems ok at first glance.

However, the header length is not just the length of the payload, but 
also the header itself.  So sizeof(*trans_hdr) should be added to 
sizeof(struct wire_msg_hdr).  Otherwise msg_hdr_len could be 64 (just 
the size of wire_msg_hdr) which is more than sizeof(*trans_hdr) but 
still means we don't have a transaction to decode.

> +	    msg_hdr_len > QAIC_MANAGE_MAX_MSG_LENGTH)
>   		return -EINVAL;
>   
>   	user_msg->len = 0;
>   	user_msg->count = le32_to_cpu(msg->hdr.count);
>   
>   	for (i = 0; i < user_msg->count; ++i) {
> +		u32 hdr_len;
> +
> +		if (msg_len >= msg_hdr_len - sizeof(*trans_hdr))
> +			return -EINVAL;
> +
>   		trans_hdr = (struct wire_trans_hdr *)(msg->data + msg_len);
> -		if (msg_len + le32_to_cpu(trans_hdr->len) > msg_hdr_len)
> +		hdr_len = le32_to_cpu(trans_hdr->len);
> +		if (hdr_len < sizeof(*trans_hdr) ||
> +		    size_add(msg_len, hdr_len) > msg_hdr_len)
>   			return -EINVAL;
>   
>   		switch (le32_to_cpu(trans_hdr->type)) {

[PATCH 3/5] accel/qaic: Add consistent integer overflow checks

[Dan Carpenter]

The encode_dma() function has integer overflow checks.  The
encode_passthrough(), encode_activate() and encode_status() functions
did not.  I added integer overflow checking everywhere.  I also
updated the integer overflow checking in encode_dma() to use size_add()
so everything is consistent.

Fixes: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/accel/qaic/qaic_control.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
index 78f6c3d6380d..96a26539df18 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -366,7 +366,7 @@ static int encode_passthrough(struct qaic_device *qdev, void *trans, struct wrap
 	if (in_trans->hdr.len % 8 != 0)
 		return -EINVAL;
 
-	if (msg_hdr_len + in_trans->hdr.len > QAIC_MANAGE_EXT_MSG_LENGTH)
+	if (size_add(msg_hdr_len, in_trans->hdr.len) > QAIC_MANAGE_EXT_MSG_LENGTH)
 		return -ENOSPC;
 
 	trans_wrapper = add_wrapper(wrappers,
@@ -557,12 +557,10 @@ static int encode_dma(struct qaic_device *qdev, void *trans, struct wrapper_list
 	msg = &wrapper->msg;
 	msg_hdr_len = le32_to_cpu(msg->hdr.len);
 
-	if (msg_hdr_len > (UINT_MAX - QAIC_MANAGE_EXT_MSG_LENGTH))
-		return -EINVAL;
-
 	/* There should be enough space to hold at least one ASP entry. */
-	if (msg_hdr_len + sizeof(*out_trans) + sizeof(struct wire_addr_size_pair) >
-	    QAIC_MANAGE_EXT_MSG_LENGTH)
+	if (size_add(msg_hdr_len,
+		     sizeof(*out_trans) + sizeof(struct wire_addr_size_pair)) >
+		     QAIC_MANAGE_EXT_MSG_LENGTH)
 		return -ENOMEM;
 
 	if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size)
@@ -634,7 +632,7 @@ static int encode_activate(struct qaic_device *qdev, void *trans, struct wrapper
 	msg = &wrapper->msg;
 	msg_hdr_len = le32_to_cpu(msg->hdr.len);
 
-	if (msg_hdr_len + sizeof(*out_trans) > QAIC_MANAGE_MAX_MSG_LENGTH)
+	if (size_add(msg_hdr_len, sizeof(*out_trans)) > QAIC_MANAGE_MAX_MSG_LENGTH)
 		return -ENOSPC;
 
 	if (!in_trans->queue_size)
@@ -718,7 +716,7 @@ static int encode_status(struct qaic_device *qdev, void *trans, struct wrapper_l
 	msg = &wrapper->msg;
 	msg_hdr_len = le32_to_cpu(msg->hdr.len);
 
-	if (msg_hdr_len + in_trans->hdr.len > QAIC_MANAGE_MAX_MSG_LENGTH)
+	if (size_add(msg_hdr_len, in_trans->hdr.len) > QAIC_MANAGE_MAX_MSG_LENGTH)
 		return -ENOSPC;
 
 	trans_wrapper = add_wrapper(wrappers, sizeof(*trans_wrapper));
-- 
2.39.2

Re: [PATCH 3/5] accel/qaic: Add consistent integer overflow checks

[Jeffrey Hugo]

On 6/21/2023 1:22 AM, Dan Carpenter wrote:
> The encode_dma() function has integer overflow checks.  The
> encode_passthrough(), encode_activate() and encode_status() functions
> did not.  I added integer overflow checking everywhere.  I also
> updated the integer overflow checking in encode_dma() to use size_add()
> so everything is consistent.
> 
> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 3/5] accel/qaic: Add consistent integer overflow checks

[Pranjal Ramajor Asha Kanojiya]

On 7/8/2023 12:21 AM, Jeffrey Hugo wrote:
> On 6/21/2023 1:22 AM, Dan Carpenter wrote:
>> The encode_dma() function has integer overflow checks.  The
>> encode_passthrough(), encode_activate() and encode_status() functions
>> did not.  I added integer overflow checking everywhere.  I also
>> updated the integer overflow checking in encode_dma() to use size_add()
>> so everything is consistent.
>>
>> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Looks good to me. Just the #include <overflow.h> as Jeff suggested.

Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>

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

[Dan Carpenter]

The integer overflow checking for find_and_map_user_pages() was done in
encode_dma().  Presumably this was to do it before the allocation.  But
it's not super important that the failure path is a fast path and it
hurts readability to put the check so far from the where the variable is
used.

Move the check to find_and_map_user_pages() instead and add some more
additional potential integer overflow checks.

Fixes: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
I kind of went to town adding integer overflow checks here.  Please,
review this extra carefully.

 drivers/accel/qaic/qaic_control.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
index 96a26539df18..03932197f1ac 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -401,6 +401,12 @@ static int find_and_map_user_pages(struct qaic_device *qdev,
 
 	xfer_start_addr = in_trans->addr + resources->xferred_dma_size;
 
+	if (in_trans->size == 0 ||
+	    in_trans->addr + in_trans->size < in_trans->addr ||
+	    in_trans->addr + resources->xferred_dma_size < in_trans->addr ||
+	    in_trans->size + offset_in_page(xfer_start_addr) < resources->xferred_dma_size)
+		return -EINVAL;
+
 	need_pages = DIV_ROUND_UP(in_trans->size + offset_in_page(xfer_start_addr) -
 				  resources->xferred_dma_size, PAGE_SIZE);
 
@@ -563,9 +569,6 @@ static int encode_dma(struct qaic_device *qdev, void *trans, struct wrapper_list
 		     QAIC_MANAGE_EXT_MSG_LENGTH)
 		return -ENOMEM;
 
-	if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size)
-		return -EINVAL;
-
 	xfer = kmalloc(sizeof(*xfer), GFP_KERNEL);
 	if (!xfer)
 		return -ENOMEM;
-- 
2.39.2

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

[Jeffrey Hugo]

On 6/21/2023 1:22 AM, Dan Carpenter wrote:
> The integer overflow checking for find_and_map_user_pages() was done in
> encode_dma().  Presumably this was to do it before the allocation.  But
> it's not super important that the failure path is a fast path and it
> hurts readability to put the check so far from the where the variable is
> used.
> 
> Move the check to find_and_map_user_pages() instead and add some more
> additional potential integer overflow checks.
> 
> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> I kind of went to town adding integer overflow checks here.  Please,
> review this extra carefully.
> 
>   drivers/accel/qaic/qaic_control.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
> index 96a26539df18..03932197f1ac 100644
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
> @@ -401,6 +401,12 @@ static int find_and_map_user_pages(struct qaic_device *qdev,
>   
>   	xfer_start_addr = in_trans->addr + resources->xferred_dma_size;
>   
> +	if (in_trans->size == 0 ||
> +	    in_trans->addr + in_trans->size < in_trans->addr ||
> +	    in_trans->addr + resources->xferred_dma_size < in_trans->addr ||
> +	    in_trans->size + offset_in_page(xfer_start_addr) < resources->xferred_dma_size)

These checks seem correct to me.
However, I wonder if it would be better to use check_add_overflow() 
instead of open coding the check?  Feels like that would make the 
purpose of the code clearer and reduce the possibility that the logic is 
wrong.

For the final check, I'm thinking that it does not need to check against 
resources->xferred_dma_size and can check against in_trans->size 
instead, which would then make the use of check_add_overflow() possible. 
  xferred_dma_size should be trusted, and should be <= size.  So then, 
the only way it would appear that check fails is addition overflow, and 
so checking against size should then be valid.

> +		return -EINVAL;
> +
>   	need_pages = DIV_ROUND_UP(in_trans->size + offset_in_page(xfer_start_addr) -
>   				  resources->xferred_dma_size, PAGE_SIZE);
>   
> @@ -563,9 +569,6 @@ static int encode_dma(struct qaic_device *qdev, void *trans, struct wrapper_list
>   		     QAIC_MANAGE_EXT_MSG_LENGTH)
>   		return -ENOMEM;
>   
> -	if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size)
> -		return -EINVAL;
> -
>   	xfer = kmalloc(sizeof(*xfer), GFP_KERNEL);
>   	if (!xfer)
>   		return -ENOMEM;

[PATCH 5/5] accel/qaic: Fix a leak in map_user_pages()

[Dan Carpenter]

If get_user_pages_fast() allocates some pages but not as many as we
wanted, then the current code leaks those pages.  Call put_page() on
the pages before returning.

Fixes: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/accel/qaic/qaic_control.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
index 03932197f1ac..7c3f9009617f 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -424,9 +424,12 @@ static int find_and_map_user_pages(struct qaic_device *qdev,
 	}
 
 	ret = get_user_pages_fast(xfer_start_addr, nr_pages, 0, page_list);
-	if (ret < 0 || ret != nr_pages) {
-		ret = -EFAULT;
+	if (ret < 0)
 		goto free_page_list;
+	if (ret != nr_pages) {
+		nr_pages = ret;
+		ret = -EFAULT;
+		goto put_pages;
 	}
 
 	sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
-- 
2.39.2

Re: [PATCH 5/5] accel/qaic: Fix a leak in map_user_pages()

[Jeffrey Hugo]

On 6/21/2023 1:22 AM, Dan Carpenter wrote:
> If get_user_pages_fast() allocates some pages but not as many as we
> wanted, then the current code leaks those pages.  Call put_page() on
> the pages before returning.
> 
> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 5/5] accel/qaic: Fix a leak in map_user_pages()

[Pranjal Ramajor Asha Kanojiya]

On 6/21/2023 12:52 PM, Dan Carpenter wrote:
> If get_user_pages_fast() allocates some pages but not as many as we
> wanted, then the current code leaks those pages.  Call put_page() on
> the pages before returning.
> 
> Fixes: 129776ac2e38 ("accel/qaic: Add control path")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>   drivers/accel/qaic/qaic_control.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
> index 03932197f1ac..7c3f9009617f 100644
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
> @@ -424,9 +424,12 @@ static int find_and_map_user_pages(struct qaic_device *qdev,
>   	}
>   
>   	ret = get_user_pages_fast(xfer_start_addr, nr_pages, 0, page_list);
> -	if (ret < 0 || ret != nr_pages) {
> -		ret = -EFAULT;
> +	if (ret < 0)
>   		goto free_page_list;
> +	if (ret != nr_pages) {
> +		nr_pages = ret;
> +		ret = -EFAULT;
> +		goto put_pages;
>   	}
>   
>   	sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);

Thank you for catching this :)

Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>

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

[Jeffrey Hugo]

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

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

[Dan Carpenter]

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