Re: [PATCH v3 2/5] drm: Add drm_get_acpi_edid() helper
On 2/2/2024 3:41 AM, Mario Limonciello wrote:
Some manufacturers have intentionally put an EDID that differs from
the EDID on the internal panel on laptops. Drivers can call this
helper to attempt to fetch the EDID from the BIOS's ACPI _DDC method.
Signed-off-by: Mario Limonciello
---
v1->v2:
* Split code from previous amdgpu specific helper to generic drm helper.
v2->v3:
* Add an extra select to fix a variety of randconfig errors found from
LKP robot.
---
drivers/gpu/drm/Kconfig| 5 +++
drivers/gpu/drm/drm_edid.c | 73 ++
include/drm/drm_edid.h | 1 +
3 files changed, 79 insertions(+)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 2520db0b776e..14df907c96c8 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -21,6 +21,11 @@ menuconfig DRM
select KCMP
select VIDEO_CMDLINE
select VIDEO_NOMODESET
+ select ACPI_VIDEO if ACPI
+ select BACKLIGHT_CLASS_DEVICE if ACPI
+ select INPUT if ACPI
+ select X86_PLATFORM_DEVICES if ACPI && X86
+ select ACPI_WMI if ACPI && X86
help
Kernel-level support for the Direct Rendering Infrastructure (DRI)
introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 69c68804023f..1fbbeaa664b2 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -28,6 +28,7 @@
* DEALINGS IN THE SOFTWARE.
*/
+#include
#include
#include
#include
@@ -2188,6 +2189,47 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int
block, size_t len)
return ret == xfers ? 0 : -1;
}
+/**
+ * drm_do_probe_acpi_edid() - get EDID information via ACPI _DDC
+ * @data: struct drm_device
+ * @buf: EDID data buffer to be filled
+ * @block: 128 byte EDID block to start fetching from
+ * @len: EDID data buffer length to fetch
+ *
+ * Try to fetch EDID information by calling acpi_video_get_edid() function.
+ *
+ * Return: 0 on success or error code on failure.
+ */
+static int
+drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t len)
+{
+ struct drm_device *ddev = data;
+ struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
+ unsigned char start = block * EDID_LENGTH;
+ void *edid;
+ int r;
+
+ if (!acpidev)
+ return -ENODEV;
+
+ /* fetch the entire edid from BIOS */
+ r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, );
+ if (r < 0) {
+ DRM_DEBUG_KMS("Failed to get EDID from ACPI: %d\n", r);
+ return -EINVAL;
+ }
+ if (len > r || start > r || start + len > r) {
+ r = EINVAL;
-EINVAL ?
+ goto cleanup;
+ }
+
+ memcpy(buf, edid + start, len);
+ r = 0;
New line before goto label?
+cleanup:
+ kfree(edid);
+ return r;
+}
+
static void connector_bad_edid(struct drm_connector *connector,
const struct edid *edid, int num_blocks)
{
@@ -2643,6 +2685,37 @@ struct edid *drm_get_edid(struct drm_connector
*connector,
}
EXPORT_SYMBOL(drm_get_edid);
+/**
+ * drm_get_acpi_edid - get EDID data, if available
+ * @connector: connector we're probing
+ *
+ * Use the BIOS to attempt to grab EDID data if possible. If found,
+ * attach it to the connector.
+ *
+ * Return: Pointer to valid EDID or NULL if we couldn't find any.
+ */
+struct edid *drm_get_acpi_edid(struct drm_connector *connector)
+{
+ struct edid *edid = NULL;
+
+ switch (connector->connector_type) {
+ case DRM_MODE_CONNECTOR_LVDS:
+ case DRM_MODE_CONNECTOR_eDP:
+ break;
+ default:
+ return NULL;
+ }
+
+ if (connector->force == DRM_FORCE_OFF)
+ return NULL;
+
+ edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid,
connector->dev, NULL);
+
+ drm_connector_update_edid_property(connector, edid);
+ return edid;
+}
+EXPORT_SYMBOL(drm_get_acpi_edid);
+
/**
* drm_edid_read_custom - Read EDID data using given EDID block read function
* @connector: Connector to use
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 518d1b8106c7..60fbdc06badc 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -412,6 +412,7 @@ struct edid *drm_do_get_edid(struct drm_connector
*connector,
void *data);
struct edid *drm_get_edid(struct drm_connector *connector,
struct i2c_adapter *adapter);
+struct edid *drm_get_acpi_edid(struct drm_connector *connector);
u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
struct i2c_adapter *adapter);
New line before return ?
Re: [PATCH v3 5/5] drm: Drop unneeded selects in DRM drivers
On 2/2/2024 3:41 AM, Mario Limonciello wrote: All of the selects on ACPI_VIDEO are unnecessary when DRM does the select for ACPI_VIDEO as it provides a helper for acpi based EDID. Signed-off-by: Mario Limonciello Reviewed-by: Pranjal Ramajor Asha Kanojiya
Re: [PATCH][V2][next] accel/qaic: remove redundant pointer pexec
On 7/26/2023 7:36 PM, Colin Ian King wrote: Pointer pexec is being assigned a value however it is never read. The assignment is redundant and can be removed. Replace sizeof(*pexec) with sizeof the type and remove the declaration of pointer pexec. Signed-off-by: Colin Ian King Reviewed-by: Pranjal Ramajor Asha Kanojiya
Re: [PATCH][next] accel/qaic: remove redundant assignment to pointer pexec
On 7/26/2023 8:30 AM, Jeffrey Hugo wrote: On 7/25/2023 5:40 AM, Colin Ian King wrote: Pointer pexec is being assigned a value however it is never read. The assignment is redundant and can be removed. Signed-off-by: Colin Ian King --- drivers/accel/qaic/qaic_data.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c index e9a1cb779b30..8a6cb14f490e 100644 --- a/drivers/accel/qaic/qaic_data.c +++ b/drivers/accel/qaic/qaic_data.c @@ -1320,7 +1320,6 @@ static int __qaic_execute_bo_ioctl(struct drm_device *dev, void *data, struct dr user_data = u64_to_user_ptr(args->data); exec = kcalloc(args->hdr.count, size, GFP_KERNEL); - pexec = (struct qaic_partial_execute_entry *)exec; if (!exec) return -ENOMEM; It does look like pexec is not used in this function after it was refactored. Shouldn't the declaration at the beginning of the function also be removed? Yeah we should remove the declaration as well. Although it is used some where to calculate its size i.e. sizeof(*pexec). We need to directly use the type in sizeof() i.e. sizeof(struct qaic_partial_execute_entry).
Re: [PATCH 5/5 v4] accel/qaic: Fix a leak in map_user_pages()
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
Reviewed-by: Pranjal Ramajor Asha Kanojiya
Re: [PATCH 4/5 v4] accel/qaic: move and expand integer overflow checks for map_user_pages()
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
---
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 3/5 v4] accel/qaic: Add consistent integer overflow checks
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
---
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 = >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 = >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 = >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
Re: [PATCH 2/5 v4] accel/qaic: tighten bounds checking in decode_message()
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
Reviewed-by: Pranjal Ramajor Asha Kanojiya
Re: [PATCH 1/5 v4] accel/qaic: tighten bounds checking in encode_message()
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
Reviewed-by: Pranjal Ramajor Asha Kanojiya
Re: [PATCH 3/5] accel/qaic: Add consistent integer overflow checks
On 7/8/2023 12:21 AM, Jeffrey Hugo wrote:
On 6/21/2023 1:22 AM, 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
Reviewed-by: Jeffrey Hugo
Looks good to me. Just the #include as Jeff suggested.
Reviewed-by: Pranjal Ramajor Asha Kanojiya
Re: [PATCH 5/5] accel/qaic: Fix a leak in map_user_pages()
On 6/21/2023 12:52 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
---
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 03932197f1ac..7c3f9009617f 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -424,9 +424,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);
Thank you for catching this :)
Reviewed-by: Pranjal Ramajor Asha Kanojiya
Re: [PATCH 1/5] accel/qaic: tighten bounds checking in encode_message()
On 7/4/2023 2:08 PM, Dan Carpenter wrote:
On Tue, Jul 04, 2023 at 11:57:51AM +0530, Pranjal Ramajor Asha Kanojiya wrote:
diff --git a/drivers/accel/qaic/qaic_control.c
b/drivers/accel/qaic/qaic_control.c
index 5c57f7b4494e..a51b1594dcfa 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct
manage_msg *user_msg,
int ret;
int i;
- if (!user_msg->count) {
+ if (!user_msg->count ||
+ user_msg->len < sizeof(*trans_hdr)) {
ret = -EINVAL;
goto out;
}
@@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev,
struct manage_msg *user_msg,
}
for (i = 0; i < user_msg->count; ++i) {
- if (user_len >= user_msg->len) {
+ if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
If I understand correctly this check is added to verify if we are left with
trans_hdr size of data. In that case '>' comparison operator should be used.
That was there in the original code and I thought about changing it but
I don't like changing things which aren't necessary and == is also
invalid so I decided to leave it.
I see, I understand your concern about not changing unnecessary things
but '>=' is incorrect for reason mentioned above. We need to change that
to '>'
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) {
If we change to > then the == will be caught by this check. So it
doesn't affect runtime either way.
I fail to see that.
Lets run an example:
user_len is 0
user_msg->len is 8
sizeof(*trans_hdr) is 8
trans_hdr->len is 8
Above instance is correct and should be processed without error.
So user_len > user_msg->len - sizeof(*trans_hdr) translates to
(0 > 8 - 8)
(0 > 0)
false (No error)
.
.
.
trans_hdr->len < sizeof(trans_hdr) ||
size_add(user_len, trans_hdr->len) > user_msg->len, translates to
8 < 8 || size_add(0, 8) > 8
false || 8 > 8
false || false
false (No error)
Am I missing anything?
regards,
dan carpenter
Re: [PATCH 1/5] accel/qaic: tighten bounds checking in encode_message()
On 7/4/2023 11:57 AM, Pranjal Ramajor Asha Kanojiya wrote:
On 6/21/2023 12:51 PM, Dan Carpenter wrote:
There are several issues in this code. The check at the start of the
loop:
if (user_len >= user_msg->len) {
This check does not ensure that we have enough space for the trans_hdr
(8 bytes). Instead the check needs to be:
if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
That subtraction is done as an unsigned long we want to avoid
negatives. Add a lower bound to the start of the function.
if (user_msg->len < sizeof(*trans_hdr))
There is a second integer underflow which can happen if
trans_hdr->len is zero inside the encode_passthrough() function.
memcpy(out_trans->data, in_trans->data, in_trans->hdr.len -
sizeof(in_trans->hdr));
Instead of adding a check to encode_passthrough() it's better to check
in this central place. Add that check:
if (trans_hdr->len < sizeof(trans_hdr)
The final concern is that the "user_len + trans_hdr->len" might have an
integer overflow bug. Use size_add() to prevent that.
- if (user_len + trans_hdr->len > user_msg->len) {
+ if (size_add(user_len, trans_hdr->len) > user_msg->len) {
Fixes: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter
---
This is based on code review and not tested.
drivers/accel/qaic/qaic_control.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/accel/qaic/qaic_control.c
b/drivers/accel/qaic/qaic_control.c
index 5c57f7b4494e..a51b1594dcfa 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -748,7 +748,8 @@ static int encode_message(struct qaic_device
*qdev, struct manage_msg *user_msg,
int ret;
int i;
- if (!user_msg->count) {
+ if (!user_msg->count ||
+ user_msg->len < sizeof(*trans_hdr)) {
ret = -EINVAL;
goto out;
}
@@ -765,12 +766,13 @@ static int encode_message(struct qaic_device
*qdev, struct manage_msg *user_msg,
}
for (i = 0; i < user_msg->count; ++i) {
- if (user_len >= user_msg->len) {
+ if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
If I understand correctly this check is added to verify if we are left
with trans_hdr size of data. In that case '>' comparison operator should
be used.
ret = -EINVAL;
break;
}
trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data
+ user_len);
- if (user_len + trans_hdr->len > user_msg->len) {
+ if (trans_hdr->len < sizeof(trans_hdr) ||
+ size_add(user_len, trans_hdr->len) > user_msg->len) {
Since the size of characters per line is 100 now. Can we rearrange this
if condition and have them in one line. Similarity at other places in
this patch series.
Thank you.
ret = -EINVAL;
break;
}
Re: [PATCH 2/5] accel/qaic: tighten bounds checking in decode_message()
On 6/21/2023 12:51 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
---
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 a51b1594dcfa..78f6c3d6380d 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -955,15 +955,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;
If I understand correctly this check is added to verify if we are left
with trans_hdr size of data. In that case '>' comparison operator should
be used.
+
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)) {
Re: [PATCH 1/5] accel/qaic: tighten bounds checking in encode_message()
On 6/21/2023 12:51 PM, Dan Carpenter wrote:
There are several issues in this code. The check at the start of the
loop:
if (user_len >= user_msg->len) {
This check does not ensure that we have enough space for the trans_hdr
(8 bytes). Instead the check needs to be:
if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
That subtraction is done as an unsigned long we want to avoid
negatives. Add a lower bound to the start of the function.
if (user_msg->len < sizeof(*trans_hdr))
There is a second integer underflow which can happen if
trans_hdr->len is zero inside the encode_passthrough() function.
memcpy(out_trans->data, in_trans->data, in_trans->hdr.len -
sizeof(in_trans->hdr));
Instead of adding a check to encode_passthrough() it's better to check
in this central place. Add that check:
if (trans_hdr->len < sizeof(trans_hdr)
The final concern is that the "user_len + trans_hdr->len" might have an
integer overflow bug. Use size_add() to prevent that.
- if (user_len + trans_hdr->len > user_msg->len) {
+ if (size_add(user_len, trans_hdr->len) > user_msg->len) {
Fixes: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter
---
This is based on code review and not tested.
drivers/accel/qaic/qaic_control.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/accel/qaic/qaic_control.c
b/drivers/accel/qaic/qaic_control.c
index 5c57f7b4494e..a51b1594dcfa 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -748,7 +748,8 @@ static int encode_message(struct qaic_device *qdev, struct
manage_msg *user_msg,
int ret;
int i;
- if (!user_msg->count) {
+ if (!user_msg->count ||
+ user_msg->len < sizeof(*trans_hdr)) {
ret = -EINVAL;
goto out;
}
@@ -765,12 +766,13 @@ static int encode_message(struct qaic_device *qdev,
struct manage_msg *user_msg,
}
for (i = 0; i < user_msg->count; ++i) {
- if (user_len >= user_msg->len) {
+ if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
If I understand correctly this check is added to verify if we are left
with trans_hdr size of data. In that case '>' comparison operator should
be used.
ret = -EINVAL;
break;
}
trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data +
user_len);
- if (user_len + trans_hdr->len > user_msg->len) {
+ if (trans_hdr->len < sizeof(trans_hdr) ||
+ size_add(user_len, trans_hdr->len) > user_msg->len) {
ret = -EINVAL;
break;
}
Re: [PATCH 1/5] accel/qaic: tighten bounds checking in encode_message()
On 6/21/2023 12:51 PM, Dan Carpenter wrote:
There are several issues in this code. The check at the start of the
loop:
if (user_len >= user_msg->len) {
This check does not ensure that we have enough space for the trans_hdr
(8 bytes). Instead the check needs to be:
if (user_len >= user_msg->len - sizeof(*trans_hdr)) {
That subtraction is done as an unsigned long we want to avoid
negatives. Add a lower bound to the start of the function.
if (user_msg->len < sizeof(*trans_hdr))
There is a second integer underflow which can happen if
trans_hdr->len is zero inside the encode_passthrough() function.
memcpy(out_trans->data, in_trans->data, in_trans->hdr.len -
sizeof(in_trans->hdr));
Instead of adding a check to encode_passthrough() it's better to check
in this central place. Add that check:
if (trans_hdr->len < sizeof(trans_hdr)
The final concern is that the "user_len + trans_hdr->len" might have an
integer overflow bug. Use size_add() to prevent that.
- if (user_len + trans_hdr->len > user_msg->len) {
+ if (size_add(user_len, trans_hdr->len) > user_msg->len) {
Fixes: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter
---
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)) {
Can we have something like this here
user_msg->len < sizeof(*trans_hdr) * user_msg->count, no?
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)) {
Do you think it is more readable if we have something like this
user_len + sizeof(*trans_hdr) >= user_msg->len
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;
}
Hey Dan, Thank you for going through qaic driver. You patches are very
much appreciated. This is good work.
Re: [PATCH] accel/qaic: Fix dereferencing freed memory
On 6/12/2023 8:39 PM, Jeffrey Hugo wrote:
On 6/12/2023 7:21 AM, Christian König wrote:
Am 12.06.23 um 15:03 schrieb Pranjal Ramajor Asha Kanojiya:
On 6/12/2023 4:52 PM, Christian König wrote:
Am 10.06.23 um 04:12 schrieb Sukrut Bellary:
smatch warning:
drivers/accel/qaic/qaic_data.c:620 qaic_free_object() error:
dereferencing freed memory 'obj->import_attach'
obj->import_attach is detached and freed using dma_buf_detach().
But used after free to decrease the dmabuf ref count using
dma_buf_put().
Fixes: ff13be830333 ("accel/qaic: Add datapath")
Signed-off-by: Sukrut Bellary
---
drivers/accel/qaic/qaic_data.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/accel/qaic/qaic_data.c
b/drivers/accel/qaic/qaic_data.c
index e42c1f98..7cba4d680ea8 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -613,11 +613,13 @@ static int qaic_gem_object_mmap(struct
drm_gem_object *obj, struct vm_area_struc
static void qaic_free_object(struct drm_gem_object *obj)
{
struct qaic_bo *bo = to_qaic_bo(obj);
+ struct dma_buf *dmabuf;
Maybe move that variable into the if.
if (obj->import_attach) {
/* DMABUF/PRIME Path */
+ dmabuf = obj->import_attach->dmabuf;
dma_buf_detach(obj->import_attach->dmabuf,
obj->import_attach);
- dma_buf_put(obj->import_attach->dmabuf);
+ dma_buf_put(dmabuf);
I strongly assume you are not using the GEM prime helpers for this?
Christian.
Driver uses drm_gem_prime_fd_to_handle() helper function but it also
registers for ->gem_prime_import() which is internally called by
drm_gem_prime_fd_to_handle(). All the operations done in
gem_prime_import() are undone here.
Then why don't you use drm_prime_gem_destroy() which is the cleanup
helper for imports created by ->gem_prime_import() ?
That looks pretty much identical to what you do here manually.
I think destroy() wasn't used because we are new to DRM and sometimes
confused by the multitude of options. I appreciate the suggestion and
will follow up to see if destroy() will work here, or identify what
would prevent the use of it.
-Jeff
Thank you Christian for your suggestion. I agree with you driver can use
drm_prime_gem_destroy() here.
Re: [PATCH] accel/qaic: Fix dereferencing freed memory
On 6/10/2023 7:42 AM, Sukrut Bellary wrote:
smatch warning:
drivers/accel/qaic/qaic_data.c:620 qaic_free_object() error:
dereferencing freed memory 'obj->import_attach'
obj->import_attach is detached and freed using dma_buf_detach().
But used after free to decrease the dmabuf ref count using
dma_buf_put().
Fixes: ff13be830333 ("accel/qaic: Add datapath")
Signed-off-by: Sukrut Bellary
---
drivers/accel/qaic/qaic_data.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index e42c1f98..7cba4d680ea8 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -613,11 +613,13 @@ static int qaic_gem_object_mmap(struct drm_gem_object
*obj, struct vm_area_struc
static void qaic_free_object(struct drm_gem_object *obj)
{
struct qaic_bo *bo = to_qaic_bo(obj);
+ struct dma_buf *dmabuf;
if (obj->import_attach) {
/* DMABUF/PRIME Path */
+ dmabuf = obj->import_attach->dmabuf;
dma_buf_detach(obj->import_attach->dmabuf, obj->import_attach);
- dma_buf_put(obj->import_attach->dmabuf);
+ dma_buf_put(dmabuf);
} else {
/* Private buffer allocation path */
qaic_free_sgt(bo->sgt);
Reviewed-by: Pranjal Ramajor Asha Kanojiya
Re: [PATCH] accel/qaic: Fix dereferencing freed memory
On 6/12/2023 4:52 PM, Christian König wrote:
Am 10.06.23 um 04:12 schrieb Sukrut Bellary:
smatch warning:
drivers/accel/qaic/qaic_data.c:620 qaic_free_object() error:
dereferencing freed memory 'obj->import_attach'
obj->import_attach is detached and freed using dma_buf_detach().
But used after free to decrease the dmabuf ref count using
dma_buf_put().
Fixes: ff13be830333 ("accel/qaic: Add datapath")
Signed-off-by: Sukrut Bellary
---
drivers/accel/qaic/qaic_data.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/accel/qaic/qaic_data.c
b/drivers/accel/qaic/qaic_data.c
index e42c1f98..7cba4d680ea8 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -613,11 +613,13 @@ static int qaic_gem_object_mmap(struct
drm_gem_object *obj, struct vm_area_struc
static void qaic_free_object(struct drm_gem_object *obj)
{
struct qaic_bo *bo = to_qaic_bo(obj);
+ struct dma_buf *dmabuf;
Maybe move that variable into the if.
if (obj->import_attach) {
/* DMABUF/PRIME Path */
+ dmabuf = obj->import_attach->dmabuf;
dma_buf_detach(obj->import_attach->dmabuf, obj->import_attach);
- dma_buf_put(obj->import_attach->dmabuf);
+ dma_buf_put(dmabuf);
I strongly assume you are not using the GEM prime helpers for this?
Christian.
Driver uses drm_gem_prime_fd_to_handle() helper function but it also
registers for ->gem_prime_import() which is internally called by
drm_gem_prime_fd_to_handle(). All the operations done in
gem_prime_import() are undone here.
} else {
/* Private buffer allocation path */
qaic_free_sgt(bo->sgt);
Re: [PATCH] MAINTAINERS: Add Carl/Pranjal as QAIC reviewers
On 5/23/2023 9:44 PM, Jeffrey Hugo wrote: Carl and Pranjal have been reviewing the QAIC patches. List them as reviewers so that they are copied on all developments which will make it easier for them to continue reviewing QAIC patches. Signed-off-by: Jeffrey Hugo --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 7e0b87d5aa2e..a0ec9ee090a4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17381,6 +17381,8 @@ F: include/dt-bindings/clock/qcom,* QUALCOMM CLOUD AI (QAIC) DRIVER M:Jeffrey Hugo +R: Carl Vanderlip +R: Pranjal Ramajor Asha Kanojiya L:linux-arm-...@vger.kernel.org L:dri-devel@lists.freedesktop.org S:Supported ACK Reviewed-by: Pranjal Ramajor Asha Kanojiya
Re: [PATCH v2] accel/qaic: initialize ret variable to 0
On 5/17/2023 10:26 PM, Jeffrey Hugo wrote:
From: Tom Rix
clang static analysis reports
drivers/accel/qaic/qaic_data.c:610:2: warning: Undefined or garbage
value returned to caller [core.uninitialized.UndefReturn]
return ret;
^~
From a code analysis of the function, the ret variable is only set some
of the time but is always returned. This suggests ret can return
uninitialized garbage. However BO allocation will ensure ret is always
set in reality.
Initialize ret to 0 to silence the warning.
Fixes: ff13be830333 ("accel/qaic: Add datapath")
Signed-off-by: Tom Rix
[jhugo: Reword commit text]
Signed-off-by: Jeffrey Hugo
---
drivers/accel/qaic/qaic_data.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index 8ab26e64b231..e42c1f98 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -591,7 +591,7 @@ static int qaic_gem_object_mmap(struct drm_gem_object *obj,
struct vm_area_struc
struct qaic_bo *bo = to_qaic_bo(obj);
unsigned long offset = 0;
struct scatterlist *sg;
- int ret;
+ int ret = 0;
if (obj->import_attach)
return -EINVAL;
Reviewed-by: Pranjal Ramajor Asha Kanojiya
Re: [PATCH] accel/qaic: silence some uninitialized variable warnings
On 5/3/2023 4:11 PM, Dan Carpenter wrote:
Smatch complains that these are not initialized if get_cntl_version()
fails but we still print them in the debug message. Not the end of
the world, but true enough. Let's just initialize them to a dummy value
to make the checker happy.
Signed-off-by: Dan Carpenter
---
drivers/accel/qaic/qaic_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index ff80eb571729..e10e8b603e37 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -263,7 +263,7 @@ static void qaic_destroy_drm_device(struct qaic_device
*qdev, s32 partition_id)
static int qaic_mhi_probe(struct mhi_device *mhi_dev, const struct
mhi_device_id *id)
{
struct qaic_device *qdev;
- u16 major, minor;
+ u16 major = -1, minor = -1;
int ret;
/*
Thank you Dan for the patch.
Reviewed-by: Pranjal Ramajor Asha Kanojiya
I agree with Jeff's comment to sort the variable declaration length wise.
