This series primarily adds check at relevant places in venus driver where there
are possible OOB accesses due to unexpected payload from venus firmware. The
patches describes the specific OOB possibility.
Please review and share your feedback.
Vikash Garodia (4):
venus: hfi: add checks to perform sanity on queue pointers
venus: hfi: fix the check to handle session buffer requirement
venus: hfi: add checks to handle capabilities from firmware
venus: hfi_parser: Add check to keep the number of codecs within range
drivers/media/platform/qcom/venus/hfi_msgs.c | 2 +-
drivers/media/platform/qcom/venus/hfi_parser.c | 27 ++++++++++++++++++++++++++
drivers/media/platform/qcom/venus/hfi_venus.c | 8 ++++++++
3 files changed, 36 insertions(+), 1 deletion(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Read and write pointers are used to track the packet index in the memory
shared between video driver and firmware. There is a possibility of OOB
access if the read or write pointer goes beyond the queue memory size.
Add checks for the read and write pointer to avoid OOB access.
Cc: [email protected]
Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
Signed-off-by: Vikash Garodia <[email protected]>
---
drivers/media/platform/qcom/venus/hfi_venus.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index f0b4638..dc228c4 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -206,6 +206,10 @@ static int venus_write_queue(struct venus_hfi_device *hdev,
new_wr_idx = wr_idx + dwords;
wr_ptr = (u32 *)(queue->qmem.kva + (wr_idx << 2));
+
+ if (wr_ptr < (u32 *)queue->qmem.kva || wr_ptr > (u32 *)(queue->qmem.kva + queue->qmem.size))
+ return -EINVAL;
+
if (new_wr_idx < qsize) {
memcpy(wr_ptr, packet, dwords << 2);
} else {
@@ -273,6 +277,10 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
}
rd_ptr = (u32 *)(queue->qmem.kva + (rd_idx << 2));
+
+ if (rd_ptr < (u32 *)queue->qmem.kva || rd_ptr > (u32 *)(queue->qmem.kva + queue->qmem.size))
+ return -EINVAL;
+
dwords = *rd_ptr >> 2;
if (!dwords)
return -EINVAL;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Buffer requirement, for different buffer type, comes from video firmware.
While copying these requirements, there is an OOB possibility when the
payload from firmware is more than expected size. Fix the check to avoid
the OOB possibility.
Cc: [email protected]
Fixes: 09c2845e8fe4 ("[media] media: venus: hfi: add Host Firmware Interface (HFI)")
Signed-off-by: Vikash Garodia <[email protected]>
---
drivers/media/platform/qcom/venus/hfi_msgs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
index 3d5dadf..3e85bd8 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -398,7 +398,7 @@ session_get_prop_buf_req(struct hfi_msg_session_property_info_pkt *pkt,
memcpy(&bufreq[idx], buf_req, sizeof(*bufreq));
idx++;
- if (idx > HFI_BUFFER_TYPE_MAX)
+ if (idx >= HFI_BUFFER_TYPE_MAX)
return HFI_ERR_SESSION_INVALID_PARAMETER;
req_bytes -= sizeof(struct hfi_buffer_requirements);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Supported codec bitmask is populated from the payload from venus firmware.
There is a possible case when all the bits in the codec bitmask is set. In
such case, core cap for decoder is filled and MAX_CODEC_NUM is utilized.
Now while filling the caps for encoder, it can lead to access the caps
array beyong 32 index. Hence leading to OOB write.
The fix counts the supported encoder and decoder. If the count is more than
max, then it skips accessing the caps.
Cc: [email protected]
Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
Signed-off-by: Vikash Garodia <[email protected]>
---
drivers/media/platform/qcom/venus/hfi_parser.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
index ec73cac..651e215 100644
--- a/drivers/media/platform/qcom/venus/hfi_parser.c
+++ b/drivers/media/platform/qcom/venus/hfi_parser.c
@@ -14,11 +14,26 @@
typedef void (*func)(struct hfi_plat_caps *cap, const void *data,
unsigned int size);
+static int count_setbits(u32 input)
+{
+ u32 count = 0;
+
+ while (input > 0) {
+ if ((input & 1) == 1)
+ count++;
+ input >>= 1;
+ }
+ return count;
+}
+
static void init_codecs(struct venus_core *core)
{
struct hfi_plat_caps *caps = core->caps, *cap;
unsigned long bit;
+ if ((count_setbits(core->dec_codecs) + count_setbits(core->enc_codecs)) > MAX_CODEC_NUM)
+ return;
+
for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
cap = &caps[core->codecs_count++];
cap->codec = BIT(bit);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 27.07.2023 06:34, Vikash Garodia wrote:
> Read and write pointers are used to track the packet index in the memory
> shared between video driver and firmware. There is a possibility of OOB
> access if the read or write pointer goes beyond the queue memory size.
> Add checks for the read and write pointer to avoid OOB access.
>
> Cc: [email protected]
> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> Signed-off-by: Vikash Garodia <[email protected]>
> ---
> drivers/media/platform/qcom/venus/hfi_venus.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index f0b4638..dc228c4 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -206,6 +206,10 @@ static int venus_write_queue(struct venus_hfi_device *hdev,
>
> new_wr_idx = wr_idx + dwords;
> wr_ptr = (u32 *)(queue->qmem.kva + (wr_idx << 2));
> +
> + if (wr_ptr < (u32 *)queue->qmem.kva || wr_ptr > (u32 *)(queue->qmem.kva + queue->qmem.size))
Shouldn't the cases on the right side of the OR operator include a
"- 1"?
Konrad
On 27.07.2023 06:34, Vikash Garodia wrote:
> Supported codec bitmask is populated from the payload from venus firmware.
> There is a possible case when all the bits in the codec bitmask is set. In
> such case, core cap for decoder is filled and MAX_CODEC_NUM is utilized.
> Now while filling the caps for encoder, it can lead to access the caps
> array beyong 32 index. Hence leading to OOB write.
> The fix counts the supported encoder and decoder. If the count is more than
> max, then it skips accessing the caps.
>
> Cc: [email protected]
> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
> Signed-off-by: Vikash Garodia <[email protected]>
> ---
> drivers/media/platform/qcom/venus/hfi_parser.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index ec73cac..651e215 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -14,11 +14,26 @@
> typedef void (*func)(struct hfi_plat_caps *cap, const void *data,
> unsigned int size);
>
> +static int count_setbits(u32 input)
hweight_long()?
Konrad
On 7/27/2023 10:38 PM, Konrad Dybcio wrote:
> On 27.07.2023 06:34, Vikash Garodia wrote:
>> Read and write pointers are used to track the packet index in the memory
>> shared between video driver and firmware. There is a possibility of OOB
>> access if the read or write pointer goes beyond the queue memory size.
>> Add checks for the read and write pointer to avoid OOB access.
>>
>> Cc: [email protected]
>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>> Signed-off-by: Vikash Garodia <[email protected]>
>> ---
>> drivers/media/platform/qcom/venus/hfi_venus.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index f0b4638..dc228c4 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -206,6 +206,10 @@ static int venus_write_queue(struct venus_hfi_device *hdev,
>>
>> new_wr_idx = wr_idx + dwords;
>> wr_ptr = (u32 *)(queue->qmem.kva + (wr_idx << 2));
>> +
>> + if (wr_ptr < (u32 *)queue->qmem.kva || wr_ptr > (u32 *)(queue->qmem.kva + queue->qmem.size))
> Shouldn't the cases on the right side of the OR operator include a
> "- 1"?
I see your point here. Possibly subtracting with sizeof(*wr_ptr) instead of "1"
would be appropriate. Similarly for read queue handling.
- Vikash
> Konrad
On Wed, Jul 26, 2023 at 9:35 PM Vikash Garodia
<[email protected]> wrote:
>
> Buffer requirement, for different buffer type, comes from video firmware.
> While copying these requirements, there is an OOB possibility when the
> payload from firmware is more than expected size. Fix the check to avoid
> the OOB possibility.
>
> Cc: [email protected]
> Fixes: 09c2845e8fe4 ("[media] media: venus: hfi: add Host Firmware Interface (HFI)")
> Signed-off-by: Vikash Garodia <[email protected]>
> ---
> drivers/media/platform/qcom/venus/hfi_msgs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
> index 3d5dadf..3e85bd8 100644
> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
> @@ -398,7 +398,7 @@ session_get_prop_buf_req(struct hfi_msg_session_property_info_pkt *pkt,
> memcpy(&bufreq[idx], buf_req, sizeof(*bufreq));
> idx++;
>
> - if (idx > HFI_BUFFER_TYPE_MAX)
> + if (idx >= HFI_BUFFER_TYPE_MAX)
> return HFI_ERR_SESSION_INVALID_PARAMETER;
>
> req_bytes -= sizeof(struct hfi_buffer_requirements);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
The fix makes sense to me.
Reviewed-by: Nathan Hebert <[email protected]>
Best regards,
Nathan Hebert