[0/4] Venus driver fixes to avoid possible OOB accesses

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(-)

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: stable@vger.kernel.org
> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>  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 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: stable@vger.kernel.org
>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>  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
<quic_vgarodia@quicinc.com> wrote:
>
> The hfi parser, parses the capabilities received from venus firmware and
> copies them to core capabilities. Consider below api, for example,
> fill_caps - In this api, caps in core structure gets updated with the
> number of capabilities received in firmware data payload. If the same api
> is called multiple times, there is a possibility of copying beyond the max
> allocated size in core caps.
> Similar possibilities in fill_raw_fmts and fill_profile_level functions.
>
> Cc: stable@vger.kernel.org
> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>  drivers/media/platform/qcom/venus/hfi_parser.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 6cf74b2..ec73cac 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -86,6 +86,9 @@ static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
>  {
>         const struct hfi_profile_level *pl = data;
>
> +       if (cap->num_pl > HFI_MAX_PROFILE_COUNT)
> +               return;
> +
This check does not fully prevent out of bounds writes. Should the
return condition be on |cap->num_pl + num > HFI_MAX_PROFILE_COUNT|, so
the last byte won't be written past the end of the array?

Similarly for the patches to |fill_caps| and |fill_raw_fmts|.
>         memcpy(&cap->pl[cap->num_pl], pl, num * sizeof(*pl));
>         cap->num_pl += num;
>  }
> @@ -111,6 +114,9 @@ fill_caps(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>  {
>         const struct hfi_capability *caps = data;
>
> +       if (cap->num_caps > MAX_CAP_ENTRIES)
> +               return;
> +
>         memcpy(&cap->caps[cap->num_caps], caps, num * sizeof(*caps));
>         cap->num_caps += num;
>  }
> @@ -137,6 +143,9 @@ static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
>  {
>         const struct raw_formats *formats = fmts;
>
> +       if (cap->num_fmts > MAX_FMT_ENTRIES)
> +               return;
> +
>         memcpy(&cap->fmts[cap->num_fmts], formats, num_fmts * sizeof(*formats));
>         cap->num_fmts += num_fmts;
>  }
> @@ -159,6 +168,9 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>                 rawfmts[i].buftype = fmt->buffer_type;
>                 i++;
>
> +               if (i >= MAX_FMT_ENTRIES)
> +                       return;
> +
>                 if (pinfo->num_planes > MAX_PLANES)
>                         break;
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Best regards,
Nathan Hebert