[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 at 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;
>>           }