| Message ID | 1691634304-2158-4-git-send-email-quic_vgarodia@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Venus driver fixes to avoid possible OOB accesses | expand |
On 10/08/2023 03:25, Vikash Garodia 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..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; > } Why append and discard though ? Couldn't we reset/reinitalise the relevant indexes in hfi_sys_init_done() ? Can subsequent notifications from the firmware give a new capability set ? Presumably not. IMO though instead of throwing away the new data, we should throw away the old data, no ? --- bod
On 8/10/2023 5:01 PM, Bryan O'Donoghue wrote: > On 10/08/2023 03:25, Vikash Garodia 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..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; >> } > > Why append and discard though ? > > Couldn't we reset/reinitalise the relevant indexes in hfi_sys_init_done() ? > > Can subsequent notifications from the firmware give a new capability set ? > Presumably not. > > IMO though instead of throwing away the new data, we should throw away the old > data, no ? 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. Regards, Vikash > > --- > bod
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 ?
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
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 ? --- bod
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(+)