* [PATCH 1/5 v4] accel/qaic: tighten bounds checking in encode_message()
2023-07-11 8:20 [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode Dan Carpenter
@ 2023-07-11 8:20 ` Dan Carpenter
2023-07-14 11:41 ` Pranjal Ramajor Asha Kanojiya
2023-07-11 8:20 ` [PATCH 2/5 v4] accel/qaic: tighten bounds checking in decode_message() Dan Carpenter
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Dan Carpenter @ 2023-07-11 8:20 UTC (permalink / raw)
To: Jeffrey Hugo
Cc: Carl Vanderlip, Pranjal Ramajor Asha Kanojiya, Oded Gabbay,
Jacek Lawrynowicz, Stanislaw Gruszka, linux-arm-msm, dri-devel,
linux-kernel, kernel-janitors
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>
---
v4: send this patch.
v2: * Fix the >= vs > bug in:
if (user_len > user_msg->len - sizeof(*trans_hdr)) {
* include overflow.h
---
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 5c57f7b4494e..2fdd5959c52f 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -14,6 +14,7 @@
#include <linux/mm.h>
#include <linux/moduleparam.h>
#include <linux/mutex.h>
+#include <linux/overflow.h>
#include <linux/pci.h>
#include <linux/scatterlist.h>
#include <linux/types.h>
@@ -748,7 +749,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 +767,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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 1/5 v4] accel/qaic: tighten bounds checking in encode_message()
2023-07-11 8:20 ` [PATCH 1/5 v4] accel/qaic: tighten bounds checking in encode_message() Dan Carpenter
@ 2023-07-14 11:41 ` Pranjal Ramajor Asha Kanojiya
2023-07-14 16:02 ` Jeffrey Hugo
0 siblings, 1 reply; 19+ messages in thread
From: Pranjal Ramajor Asha Kanojiya @ 2023-07-14 11:41 UTC (permalink / raw)
To: Dan Carpenter, Jeffrey Hugo
Cc: Carl Vanderlip, Oded Gabbay, Jacek Lawrynowicz, Stanislaw Gruszka,
linux-arm-msm, dri-devel, linux-kernel, kernel-janitors
On 7/11/2023 1:50 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>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/5 v4] accel/qaic: tighten bounds checking in encode_message()
2023-07-14 11:41 ` Pranjal Ramajor Asha Kanojiya
@ 2023-07-14 16:02 ` Jeffrey Hugo
0 siblings, 0 replies; 19+ messages in thread
From: Jeffrey Hugo @ 2023-07-14 16:02 UTC (permalink / raw)
To: Pranjal Ramajor Asha Kanojiya, Dan Carpenter
Cc: Carl Vanderlip, Oded Gabbay, Jacek Lawrynowicz, Stanislaw Gruszka,
linux-arm-msm, dri-devel, linux-kernel, kernel-janitors
On 7/14/2023 5:41 AM, Pranjal Ramajor Asha Kanojiya wrote:
>
>
> On 7/11/2023 1:50 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>
>
> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
>
Applied to drm-misc-fixes
-Jeff
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/5 v4] accel/qaic: tighten bounds checking in decode_message()
2023-07-11 8:20 [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode Dan Carpenter
2023-07-11 8:20 ` [PATCH 1/5 v4] accel/qaic: tighten bounds checking in encode_message() Dan Carpenter
@ 2023-07-11 8:20 ` Dan Carpenter
2023-07-14 11:42 ` Pranjal Ramajor Asha Kanojiya
2023-07-11 8:21 ` [PATCH 3/5 v4] accel/qaic: Add consistent integer overflow checks Dan Carpenter
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Dan Carpenter @ 2023-07-11 8:20 UTC (permalink / raw)
To: Jeffrey Hugo
Cc: Carl Vanderlip, Pranjal Ramajor Asha Kanojiya, Oded Gabbay,
Jacek Lawrynowicz, Stanislaw Gruszka, linux-arm-msm, dri-devel,
kernel-janitors
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>
---
v2: Fix the >= vs > bug in "if (msg_len > msg_hdr_len - sizeof(*trans_hdr))"
---
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 2fdd5959c52f..752b67aff777 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -956,15 +956,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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/5 v4] accel/qaic: tighten bounds checking in decode_message()
2023-07-11 8:20 ` [PATCH 2/5 v4] accel/qaic: tighten bounds checking in decode_message() Dan Carpenter
@ 2023-07-14 11:42 ` Pranjal Ramajor Asha Kanojiya
2023-07-14 16:05 ` Jeffrey Hugo
0 siblings, 1 reply; 19+ messages in thread
From: Pranjal Ramajor Asha Kanojiya @ 2023-07-14 11:42 UTC (permalink / raw)
To: Dan Carpenter, Jeffrey Hugo
Cc: Carl Vanderlip, Oded Gabbay, Jacek Lawrynowicz, Stanislaw Gruszka,
linux-arm-msm, dri-devel, kernel-janitors
On 7/11/2023 1:50 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>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/5 v4] accel/qaic: tighten bounds checking in decode_message()
2023-07-14 11:42 ` Pranjal Ramajor Asha Kanojiya
@ 2023-07-14 16:05 ` Jeffrey Hugo
0 siblings, 0 replies; 19+ messages in thread
From: Jeffrey Hugo @ 2023-07-14 16:05 UTC (permalink / raw)
To: Pranjal Ramajor Asha Kanojiya, Dan Carpenter
Cc: Carl Vanderlip, Oded Gabbay, Jacek Lawrynowicz, Stanislaw Gruszka,
linux-arm-msm, dri-devel, kernel-janitors
On 7/14/2023 5:42 AM, Pranjal Ramajor Asha Kanojiya wrote:
>
>
> On 7/11/2023 1:50 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>
>
> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Pushed to drm-misc-fixes
-Jeff
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/5 v4] accel/qaic: Add consistent integer overflow checks
2023-07-11 8:20 [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode Dan Carpenter
2023-07-11 8:20 ` [PATCH 1/5 v4] accel/qaic: tighten bounds checking in encode_message() Dan Carpenter
2023-07-11 8:20 ` [PATCH 2/5 v4] accel/qaic: tighten bounds checking in decode_message() Dan Carpenter
@ 2023-07-11 8:21 ` Dan Carpenter
2023-07-14 11:44 ` Pranjal Ramajor Asha Kanojiya
2023-07-11 8:21 ` [PATCH 4/5 v4] accel/qaic: move and expand integer overflow checks for map_user_pages() Dan Carpenter
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Dan Carpenter @ 2023-07-11 8:21 UTC (permalink / raw)
To: Jeffrey Hugo
Cc: Carl Vanderlip, Pranjal Ramajor Asha Kanojiya, Oded Gabbay,
Jacek Lawrynowicz, Stanislaw Gruszka, linux-arm-msm, dri-devel,
kernel-janitors
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)) >
+ 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));
--
2.39.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/5 v4] accel/qaic: Add consistent integer overflow checks
2023-07-11 8:21 ` [PATCH 3/5 v4] accel/qaic: Add consistent integer overflow checks Dan Carpenter
@ 2023-07-14 11:44 ` Pranjal Ramajor Asha Kanojiya
2023-07-14 16:14 ` Jeffrey Hugo
0 siblings, 1 reply; 19+ messages in thread
From: Pranjal Ramajor Asha Kanojiya @ 2023-07-14 11:44 UTC (permalink / raw)
To: Dan Carpenter, Jeffrey Hugo
Cc: Carl Vanderlip, Oded Gabbay, Jacek Lawrynowicz, Stanislaw Gruszka,
linux-arm-msm, dri-devel, kernel-janitors
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>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/5 v4] accel/qaic: Add consistent integer overflow checks
2023-07-14 11:44 ` Pranjal Ramajor Asha Kanojiya
@ 2023-07-14 16:14 ` Jeffrey Hugo
0 siblings, 0 replies; 19+ messages in thread
From: Jeffrey Hugo @ 2023-07-14 16:14 UTC (permalink / raw)
To: Pranjal Ramajor Asha Kanojiya, Dan Carpenter
Cc: Carl Vanderlip, Oded Gabbay, Jacek Lawrynowicz, Stanislaw Gruszka,
linux-arm-msm, dri-devel, kernel-janitors
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/5 v4] accel/qaic: move and expand integer overflow checks for map_user_pages()
2023-07-11 8:20 [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode Dan Carpenter
` (2 preceding siblings ...)
2023-07-11 8:21 ` [PATCH 3/5 v4] accel/qaic: Add consistent integer overflow checks Dan Carpenter
@ 2023-07-11 8:21 ` Dan Carpenter
2023-07-14 11:46 ` Pranjal Ramajor Asha Kanojiya
2023-07-11 8:21 ` [PATCH 5/5 v4] accel/qaic: Fix a leak in map_user_pages() Dan Carpenter
2023-07-11 17:33 ` [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode Jeffrey Hugo
5 siblings, 1 reply; 19+ messages in thread
From: Dan Carpenter @ 2023-07-11 8:21 UTC (permalink / raw)
To: Jeffrey Hugo
Cc: Carl Vanderlip, Pranjal Ramajor Asha Kanojiya, Oded Gabbay,
Jacek Lawrynowicz, linux-arm-msm, dri-devel, kernel-janitors
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>
---
no change
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 23680f5f1902..d5ce36cb351f 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -402,6 +402,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);
@@ -564,9 +570,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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/5 v4] accel/qaic: move and expand integer overflow checks for map_user_pages()
2023-07-11 8:21 ` [PATCH 4/5 v4] accel/qaic: move and expand integer overflow checks for map_user_pages() Dan Carpenter
@ 2023-07-14 11:46 ` Pranjal Ramajor Asha Kanojiya
0 siblings, 0 replies; 19+ messages in thread
From: Pranjal Ramajor Asha Kanojiya @ 2023-07-14 11:46 UTC (permalink / raw)
To: Dan Carpenter, Jeffrey Hugo
Cc: Carl Vanderlip, Oded Gabbay, Jacek Lawrynowicz, linux-arm-msm,
dri-devel, kernel-janitors
On 7/11/2023 1:51 PM, 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>
> ---
> no change
>
> 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 23680f5f1902..d5ce36cb351f 100644
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
> @@ -402,6 +402,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 ||
Why not use xfer_start_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);
>
> @@ -564,9 +570,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;
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/5 v4] accel/qaic: Fix a leak in map_user_pages()
2023-07-11 8:20 [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode Dan Carpenter
` (3 preceding siblings ...)
2023-07-11 8:21 ` [PATCH 4/5 v4] accel/qaic: move and expand integer overflow checks for map_user_pages() Dan Carpenter
@ 2023-07-11 8:21 ` Dan Carpenter
2023-07-14 11:47 ` Pranjal Ramajor Asha Kanojiya
2023-07-11 17:33 ` [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode Jeffrey Hugo
5 siblings, 1 reply; 19+ messages in thread
From: Dan Carpenter @ 2023-07-11 8:21 UTC (permalink / raw)
To: Jeffrey Hugo
Cc: Carl Vanderlip, Pranjal Ramajor Asha Kanojiya, Oded Gabbay,
Jacek Lawrynowicz, linux-arm-msm, dri-devel, kernel-janitors
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>
---
no change
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 d5ce36cb351f..9a6f80f31c65 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -425,9 +425,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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/5 v4] accel/qaic: Fix a leak in map_user_pages()
2023-07-11 8:21 ` [PATCH 5/5 v4] accel/qaic: Fix a leak in map_user_pages() Dan Carpenter
@ 2023-07-14 11:47 ` Pranjal Ramajor Asha Kanojiya
2023-07-14 16:17 ` Jeffrey Hugo
0 siblings, 1 reply; 19+ messages in thread
From: Pranjal Ramajor Asha Kanojiya @ 2023-07-14 11:47 UTC (permalink / raw)
To: Dan Carpenter, Jeffrey Hugo
Cc: Carl Vanderlip, Oded Gabbay, Jacek Lawrynowicz, linux-arm-msm,
dri-devel, kernel-janitors
On 7/11/2023 1:51 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>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 5/5 v4] accel/qaic: Fix a leak in map_user_pages()
2023-07-14 11:47 ` Pranjal Ramajor Asha Kanojiya
@ 2023-07-14 16:17 ` Jeffrey Hugo
0 siblings, 0 replies; 19+ messages in thread
From: Jeffrey Hugo @ 2023-07-14 16:17 UTC (permalink / raw)
To: Pranjal Ramajor Asha Kanojiya, Dan Carpenter
Cc: Carl Vanderlip, Oded Gabbay, Jacek Lawrynowicz, linux-arm-msm,
dri-devel, kernel-janitors
On 7/14/2023 5:47 AM, Pranjal Ramajor Asha Kanojiya wrote:
>
>
> On 7/11/2023 1:51 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>
>
> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Pushed to drm-misc-fixes
-Jeff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode
2023-07-11 8:20 [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode Dan Carpenter
` (4 preceding siblings ...)
2023-07-11 8:21 ` [PATCH 5/5 v4] accel/qaic: Fix a leak in map_user_pages() Dan Carpenter
@ 2023-07-11 17:33 ` Jeffrey Hugo
2023-07-12 6:30 ` Dan Carpenter
5 siblings, 1 reply; 19+ messages in thread
From: Jeffrey Hugo @ 2023-07-11 17:33 UTC (permalink / raw)
To: Dan Carpenter
Cc: Carl Vanderlip, Pranjal Ramajor Asha Kanojiya, Oded Gabbay,
linux-arm-msm, dri-devel, kernel-janitors
On 7/11/2023 2:20 AM, Dan Carpenter wrote:
> Fixed in v4: Send the correct [PATCH 1/5] patch.
>
> Fixed in v3: Redo messed up threading
>
> Fixed two things in v2: Include the <linux/overflow.h> file. Change
> the >= in encode and decode to >.
>
> regards,
> dan carpenter
Did you intentionally drop tags from previous versions?
For 1-3, 5
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Looks like 3,5 are reviewed by Pranjal and also good. I see 5 is also
reviewed by Dafna. Expect those to be merged. 1,2 need a review from
Pranjal, but I expect all is good and will be merged.
I did not see feedback on my question for 4. Would like your feedback
before queuing that one up.
Overall, thanks for your work. I think we are pretty close to wrapping
this up.
-Jeff
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode
2023-07-11 17:33 ` [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode Jeffrey Hugo
@ 2023-07-12 6:30 ` Dan Carpenter
2023-07-12 14:22 ` Jeffrey Hugo
2023-08-04 14:36 ` Jeffrey Hugo
0 siblings, 2 replies; 19+ messages in thread
From: Dan Carpenter @ 2023-07-12 6:30 UTC (permalink / raw)
To: Jeffrey Hugo
Cc: Carl Vanderlip, Pranjal Ramajor Asha Kanojiya, Oded Gabbay,
linux-arm-msm, dri-devel, kernel-janitors
On Tue, Jul 11, 2023 at 11:33:25AM -0600, Jeffrey Hugo wrote:
> On 7/11/2023 2:20 AM, Dan Carpenter wrote:
> > Fixed in v4: Send the correct [PATCH 1/5] patch.
> >
> > Fixed in v3: Redo messed up threading
> >
> > Fixed two things in v2: Include the <linux/overflow.h> file. Change
> > the >= in encode and decode to >.
> >
> > regards,
> > dan carpenter
>
> Did you intentionally drop tags from previous versions?
Sorry, I kept messing up the resends.
>
> For 1-3, 5
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>
> Looks like 3,5 are reviewed by Pranjal and also good. I see 5 is also
> reviewed by Dafna. Expect those to be merged. 1,2 need a review from
> Pranjal, but I expect all is good and will be merged.
>
> I did not see feedback on my question for 4. Would like your feedback
> before queuing that one up.
>
Sorry, again. Yeah. I think you're right. Could we queue the rest and
I will resend 4 separately? I know it's a headache. If not it's fine,
I can resend them all.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode
2023-07-12 6:30 ` Dan Carpenter
@ 2023-07-12 14:22 ` Jeffrey Hugo
2023-08-04 14:36 ` Jeffrey Hugo
1 sibling, 0 replies; 19+ messages in thread
From: Jeffrey Hugo @ 2023-07-12 14:22 UTC (permalink / raw)
To: Dan Carpenter
Cc: Carl Vanderlip, Pranjal Ramajor Asha Kanojiya, Oded Gabbay,
linux-arm-msm, dri-devel, kernel-janitors
On 7/12/2023 12:30 AM, Dan Carpenter wrote:
> On Tue, Jul 11, 2023 at 11:33:25AM -0600, Jeffrey Hugo wrote:
>> On 7/11/2023 2:20 AM, Dan Carpenter wrote:
>>> Fixed in v4: Send the correct [PATCH 1/5] patch.
>>>
>>> Fixed in v3: Redo messed up threading
>>>
>>> Fixed two things in v2: Include the <linux/overflow.h> file. Change
>>> the >= in encode and decode to >.
>>>
>>> regards,
>>> dan carpenter
>>
>> Did you intentionally drop tags from previous versions?
>
> Sorry, I kept messing up the resends.
>
>>
>> For 1-3, 5
>> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>>
>> Looks like 3,5 are reviewed by Pranjal and also good. I see 5 is also
>> reviewed by Dafna. Expect those to be merged. 1,2 need a review from
>> Pranjal, but I expect all is good and will be merged.
>>
>> I did not see feedback on my question for 4. Would like your feedback
>> before queuing that one up.
>>
>
> Sorry, again. Yeah. I think you're right. Could we queue the rest and
> I will resend 4 separately? I know it's a headache. If not it's fine,
> I can resend them all.
These all seem independent enough that I don't see splitting out 4 as a
problem.
-Jeff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/5 v4] accel/qaic: Improve bounds checking in encode/decode
2023-07-12 6:30 ` Dan Carpenter
2023-07-12 14:22 ` Jeffrey Hugo
@ 2023-08-04 14:36 ` Jeffrey Hugo
1 sibling, 0 replies; 19+ messages in thread
From: Jeffrey Hugo @ 2023-08-04 14:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: Carl Vanderlip, Pranjal Ramajor Asha Kanojiya, Oded Gabbay,
linux-arm-msm, dri-devel, kernel-janitors
On 7/12/2023 12:30 AM, Dan Carpenter wrote:
> On Tue, Jul 11, 2023 at 11:33:25AM -0600, Jeffrey Hugo wrote:
>> On 7/11/2023 2:20 AM, Dan Carpenter wrote:
>>> Fixed in v4: Send the correct [PATCH 1/5] patch.
>>>
>>> Fixed in v3: Redo messed up threading
>>>
>>> Fixed two things in v2: Include the <linux/overflow.h> file. Change
>>> the >= in encode and decode to >.
>>>
>>> regards,
>>> dan carpenter
>>
>> Did you intentionally drop tags from previous versions?
>
> Sorry, I kept messing up the resends.
>
>>
>> For 1-3, 5
>> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>>
>> Looks like 3,5 are reviewed by Pranjal and also good. I see 5 is also
>> reviewed by Dafna. Expect those to be merged. 1,2 need a review from
>> Pranjal, but I expect all is good and will be merged.
>>
>> I did not see feedback on my question for 4. Would like your feedback
>> before queuing that one up.
>>
>
> Sorry, again. Yeah. I think you're right. Could we queue the rest and
> I will resend 4 separately? I know it's a headache. If not it's fine,
> I can resend them all.
Resend for #4 is still coming, right? I just don't want to forget about it.
-Jeff
^ permalink raw reply [flat|nested] 19+ messages in thread