| Message ID | 1691634304-2158-4-git-send-email-quic_vgarodia@quicinc.com |
|---|---|
| State | Accepted |
| Commit | 8d0b89398b7ebc52103e055bf36b60b045f5258f |
| Headers | show |
| Series | Venus driver fixes to avoid possible OOB accesses | expand |
On 8/11/2023 4:09 PM, Bryan O'Donoghue wrote: > On 11/08/2023 09:51, Vikash Garodia wrote: >> >> On 8/11/2023 2:11 PM, Bryan O'Donoghue wrote: >>> On 11/08/2023 06:54, Vikash Garodia wrote: >>>> The case is all about rogue firmware. If there is a need to fill the same cap >>>> again, that itself indicates that the payload from firmware is not correct. In >>>> such cases, the old as well as new cap data are not reliable. Though the >>>> authenticity of the data cannot be ensured, the check would avoid any OOB >>>> during >>>> such rogue firmware case. >>> >>> Then why favour the old cap report over the new ? >> >> When the driver hits the case for OOB, thats when it knows that something has >> gone wrong. Keeping old or new, both are invalid values in such case, nothing to >> favor any value. >> >> Regards, >> Vikash > > Is this hypothetical or a real bug you are actually working to mitigate ? These are theoretical bugs, not reported during any video usecase so far. At the same time, these are quite possible when the packets from firmware goes different than expected. > --- > bod
diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c index 6cf74b2..9d6ba22 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 + num >= HFI_MAX_PROFILE_COUNT) + return; + 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 + num >= 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 + 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 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(+)