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

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/5 v3] accel/qaic: Improve bounds checking in encode/decode
@ 2023-07-11  6:08 ` Dan Carpenter
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:08 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: Carl Vanderlip, Pranjal Ramajor Asha Kanojiya, Oded Gabbay,
	linux-arm-msm, dri-devel, kernel-janitors

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 0/5 v3] accel/qaic: Improve bounds checking in encode/decode
@ 2023-07-11  6:08 ` Dan Carpenter
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:08 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: linux-arm-msm, Oded Gabbay, kernel-janitors, dri-devel,
	Pranjal Ramajor Asha Kanojiya, Carl Vanderlip

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/5 v3] accel/qaic: tighten bounds checking in encode_message()
  2023-07-11  6:08 ` Dan Carpenter
@ 2023-07-11  6:10   ` Dan Carpenter
  -1 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:10 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

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 1/5 v3] accel/qaic: tighten bounds checking in encode_message()
@ 2023-07-11  6:10   ` Dan Carpenter
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:10 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: linux-arm-msm, Oded Gabbay, kernel-janitors, dri-devel,
	Pranjal Ramajor Asha Kanojiya, Stanislaw Gruszka, Carl Vanderlip,
	Jacek Lawrynowicz

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5 v3] accel/qaic: tighten bounds checking in encode_message()
  2023-07-11  6:10   ` Dan Carpenter
@ 2023-07-11  6:20     ` Dan Carpenter
  -1 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6: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

NAK.

Wow.  I really screwed this up.  I didn't mean to send this patch.  I
have a correct patch still sitting in my postponed messages.

I will send v4 in twenty minutes.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5 v3] accel/qaic: tighten bounds checking in encode_message()
@ 2023-07-11  6:20     ` Dan Carpenter
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:20 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: linux-arm-msm, Oded Gabbay, kernel-janitors, dri-devel,
	Pranjal Ramajor Asha Kanojiya, Stanislaw Gruszka, Carl Vanderlip,
	Jacek Lawrynowicz

NAK.

Wow.  I really screwed this up.  I didn't mean to send this patch.  I
have a correct patch still sitting in my postponed messages.

I will send v4 in twenty minutes.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/5 v3] accel/qaic: tighten bounds checking in decode_message()
  2023-07-11  6:08 ` Dan Carpenter
@ 2023-07-11  6:12   ` Dan Carpenter
  -1 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:12 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] 14+ messages in thread

* [PATCH 2/5 v3] accel/qaic: tighten bounds checking in decode_message()
@ 2023-07-11  6:12   ` Dan Carpenter
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:12 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: linux-arm-msm, Oded Gabbay, kernel-janitors, dri-devel,
	Pranjal Ramajor Asha Kanojiya, Stanislaw Gruszka, Carl Vanderlip,
	Jacek Lawrynowicz

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] 14+ messages in thread

* [PATCH 3/5 v3] accel/qaic: Add consistent integer overflow checks
  2023-07-11  6:08 ` Dan Carpenter
@ 2023-07-11  6:12   ` Dan Carpenter
  -1 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:12 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] 14+ messages in thread

* [PATCH 3/5 v3] accel/qaic: Add consistent integer overflow checks
@ 2023-07-11  6:12   ` Dan Carpenter
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:12 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: linux-arm-msm, Oded Gabbay, kernel-janitors, dri-devel,
	Pranjal Ramajor Asha Kanojiya, Stanislaw Gruszka, Carl Vanderlip,
	Jacek Lawrynowicz

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] 14+ messages in thread

* [PATCH 5/5 v3] accel/qaic: Fix a leak in map_user_pages()
  2023-07-11  6:08 ` Dan Carpenter
@ 2023-07-11  6:13   ` Dan Carpenter
  -1 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:13 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] 14+ messages in thread

* [PATCH 5/5 v3] accel/qaic: Fix a leak in map_user_pages()
@ 2023-07-11  6:13   ` Dan Carpenter
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-07-11  6:13 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: linux-arm-msm, Oded Gabbay, kernel-janitors, dri-devel,
	Pranjal Ramajor Asha Kanojiya, Carl Vanderlip, Jacek Lawrynowicz

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] 14+ messages in thread

* Re: [PATCH 5/5 v3] accel/qaic: Fix a leak in map_user_pages()
  2023-07-11  6:13   ` Dan Carpenter
@ 2023-07-11  7:23     ` Dafna Hirschfeld
  -1 siblings, 0 replies; 14+ messages in thread
From: Dafna Hirschfeld @ 2023-07-11  7:23 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jeffrey Hugo, Carl Vanderlip, Pranjal Ramajor Asha Kanojiya,
	Oded Gabbay, Jacek Lawrynowicz, linux-arm-msm, dri-devel,
	kernel-janitors

On 11.07.2023 09:13, 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: Dafna Hirschfeld <dhirschfeld@habana.ai>

>---
>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	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5 v3] accel/qaic: Fix a leak in map_user_pages()
@ 2023-07-11  7:23     ` Dafna Hirschfeld
  0 siblings, 0 replies; 14+ messages in thread
From: Dafna Hirschfeld @ 2023-07-11  7:23 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jeffrey Hugo, linux-arm-msm, Oded Gabbay, kernel-janitors,
	dri-devel, Pranjal Ramajor Asha Kanojiya, Carl Vanderlip,
	Jacek Lawrynowicz

On 11.07.2023 09:13, 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: Dafna Hirschfeld <dhirschfeld@habana.ai>

>---
>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	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2023-07-11  8:01 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-11  6:08 [PATCH 0/5 v3] accel/qaic: Improve bounds checking in encode/decode Dan Carpenter
2023-07-11  6:08 ` Dan Carpenter
2023-07-11  6:10 ` [PATCH 1/5 v3] accel/qaic: tighten bounds checking in encode_message() Dan Carpenter
2023-07-11  6:10   ` Dan Carpenter
2023-07-11  6:20   ` Dan Carpenter
2023-07-11  6:20     ` Dan Carpenter
2023-07-11  6:12 ` [PATCH 2/5 v3] accel/qaic: tighten bounds checking in decode_message() Dan Carpenter
2023-07-11  6:12   ` Dan Carpenter
2023-07-11  6:12 ` [PATCH 3/5 v3] accel/qaic: Add consistent integer overflow checks Dan Carpenter
2023-07-11  6:12   ` Dan Carpenter
2023-07-11  6:13 ` [PATCH 5/5 v3] accel/qaic: Fix a leak in map_user_pages() Dan Carpenter
2023-07-11  6:13   ` Dan Carpenter
2023-07-11  7:23   ` Dafna Hirschfeld
2023-07-11  7:23     ` Dafna Hirschfeld

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.