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

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

[Dan Carpenter]

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

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

[Dan Carpenter]

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

[PATCH 1/5 v4] 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>
---
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

[PATCH 1/5 v4] 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>
---
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

[PATCH 2/5 v4] 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>
---
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

[PATCH 2/5 v4] 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>
---
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

[PATCH 3/5 v4] 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>
---
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

[PATCH 3/5 v4] 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>
---
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

[PATCH 4/5 v4] 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>
---
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

[PATCH 4/5 v4] 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>
---
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

[PATCH 5/5 v4] 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>
---
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

[PATCH 5/5 v4] 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>
---
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

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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

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

[Dan Carpenter]

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

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

[Dan Carpenter]

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

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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

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

[Pranjal Ramajor Asha Kanojiya]

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>

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

[Pranjal Ramajor Asha Kanojiya]

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>

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

[Pranjal Ramajor Asha Kanojiya]

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>

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

[Pranjal Ramajor Asha Kanojiya]

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>

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

[Pranjal Ramajor Asha Kanojiya]

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>

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

[Pranjal Ramajor Asha Kanojiya]

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>

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

[Pranjal Ramajor Asha Kanojiya]

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;

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

[Pranjal Ramajor Asha Kanojiya]

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;

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

[Pranjal Ramajor Asha Kanojiya]

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>

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

[Pranjal Ramajor Asha Kanojiya]

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>

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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

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

[Jeffrey Hugo]

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