diff mbox series

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

Message ID ZK0Q7IsPkj6WSCcL@moroto
State Accepted
Commit 47d87f71d00b7091b43a56f608f7151b33e5772e
Headers show
Series [1/5,v4] accel/qaic: tighten bounds checking in encode_message() | expand

Commit Message

Dan Carpenter July 11, 2023, 8:21 a.m. UTC
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>
---
v2: no change

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

Comments

Pranjal Ramajor Asha Kanojiya July 14, 2023, 11:44 a.m. UTC | #1
On 7/11/2023 1:51 PM, 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>
> ---
> v2: no change
> 
>   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 752b67aff777..23680f5f1902 100644
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
> @@ -367,7 +367,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,
> @@ -558,12 +558,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)) >

Can we have the entire size_add() on same line?

> +		     QAIC_MANAGE_EXT_MSG_LENGTH)
>   		return -ENOMEM;
>   
>   	if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size)
> @@ -635,7 +633,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)
> @@ -719,7 +717,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));

Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Jeffrey Hugo July 14, 2023, 4:14 p.m. UTC | #2
On 7/14/2023 5:44 AM, Pranjal Ramajor Asha Kanojiya wrote:
> 
> 
> On 7/11/2023 1:51 PM, 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>
>> ---
>> v2: no change
>>
>>   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 752b67aff777..23680f5f1902 100644
>> --- a/drivers/accel/qaic/qaic_control.c
>> +++ b/drivers/accel/qaic/qaic_control.c
>> @@ -367,7 +367,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,
>> @@ -558,12 +558,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)) >
> 
> Can we have the entire size_add() on same line?
> 
>> +             QAIC_MANAGE_EXT_MSG_LENGTH)
>>           return -ENOMEM;
>>       if (in_trans->addr + in_trans->size < in_trans->addr || 
>> !in_trans->size)
>> @@ -635,7 +633,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)
>> @@ -719,7 +717,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));
> 
> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>

Pushed to drm-misc-fixes

-Jeff
diff mbox series

Patch

diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
index 752b67aff777..23680f5f1902 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -367,7 +367,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,
@@ -558,12 +558,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)
@@ -635,7 +633,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)
@@ -719,7 +717,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));