| Message ID | 20241105-venus_oob-v1-1-8d4feedfe2bb@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] media: venus: hfi_parser: add check to avoid out of bound access | expand |
On 11/5/2024 4:21 PM, Bryan O'Donoghue wrote: > On 05/11/2024 08:54, Vikash Garodia wrote: >> There is a possibility that init_codecs is invoked multiple times during >> manipulated payload from video firmware. In such case, if codecs_count >> can get incremented to value more than MAX_CODEC_NUM, there can be OOB >> access. Keep a check for max accessible memory before accessing it. >> >> 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 | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c >> b/drivers/media/platform/qcom/venus/hfi_parser.c >> index >> 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..27d0172294d5154f4839e8cef172f9a619dfa305 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c >> @@ -23,6 +23,8 @@ static void init_codecs(struct venus_core *core) >> return; >> for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) { >> + if (core->codecs_count >= MAX_CODEC_NUM) >> + return; >> cap = &caps[core->codecs_count++]; >> cap->codec = BIT(bit); >> cap->domain = VIDC_SESSION_TYPE_DEC; >> @@ -30,6 +32,8 @@ static void init_codecs(struct venus_core *core) >> } >> for_each_set_bit(bit, &core->enc_codecs, MAX_CODEC_NUM) { >> + if (core->codecs_count >= MAX_CODEC_NUM) >> + return; >> cap = &caps[core->codecs_count++]; >> cap->codec = BIT(bit); >> cap->domain = VIDC_SESSION_TYPE_ENC; >> > > I don't see how codecs_count could be greater than the control, since you > increment by one on each loop but >= is fine too I suppose. Assume the payload from malicious firmware is packed like below HFI_PROPERTY_PARAM_CODEC_SUPPORTED HFI_PROPERTY_PARAM_CODEC_SUPPORTED HFI_PROPERTY_PARAM_CODEC_SUPPORTED ..... for 32 or more instances of above type Regards, Vikash > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On 06/11/2024 07:25, Vikash Garodia wrote: >>> cap = &caps[core->codecs_count++]; >>> cap->codec = BIT(bit); >>> cap->domain = VIDC_SESSION_TYPE_ENC; >>> >> I don't see how codecs_count could be greater than the control, since you >> increment by one on each loop but >= is fine too I suppose. > Assume the payload from malicious firmware is packed like below > HFI_PROPERTY_PARAM_CODEC_SUPPORTED > HFI_PROPERTY_PARAM_CODEC_SUPPORTED > HFI_PROPERTY_PARAM_CODEC_SUPPORTED > ..... > for 32 or more instances of above type But you do this cap = &caps[core->codecs_count++]; for each bit. Anyway consider Dmitry's input re only calling this function once instead. --- bod
On 11/5/2024 7:25 PM, Dmitry Baryshkov wrote: > On Tue, Nov 05, 2024 at 02:24:54PM +0530, Vikash Garodia wrote: >> There is a possibility that init_codecs is invoked multiple times during >> manipulated payload from video firmware. In such case, if codecs_count >> can get incremented to value more than MAX_CODEC_NUM, there can be OOB >> access. Keep a check for max accessible memory before accessing it. > > No. Please make sure that init_codecs() does a correct thing, so that > core->codecs_count isn't incremented that much (or even better that > init_codecs() doesn't do anything if it is executed second time). init_codecs() parses the payload received from firmware and . I don't think we can control this part when we have something like this from a malicious firmware payload HFI_PROPERTY_PARAM_CODEC_SUPPORTED HFI_PROPERTY_PARAM_CODEC_SUPPORTED HFI_PROPERTY_PARAM_CODEC_SUPPORTED ... Limiting it to second iteration would restrict the functionality when property HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs. Regards, Vikash >> >> 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 | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c >> index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..27d0172294d5154f4839e8cef172f9a619dfa305 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c >> @@ -23,6 +23,8 @@ static void init_codecs(struct venus_core *core) >> return; >> >> for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) { >> + if (core->codecs_count >= MAX_CODEC_NUM) >> + return; >> cap = &caps[core->codecs_count++]; >> cap->codec = BIT(bit); >> cap->domain = VIDC_SESSION_TYPE_DEC; >> @@ -30,6 +32,8 @@ static void init_codecs(struct venus_core *core) >> } >> >> for_each_set_bit(bit, &core->enc_codecs, MAX_CODEC_NUM) { >> + if (core->codecs_count >= MAX_CODEC_NUM) >> + return; >> cap = &caps[core->codecs_count++]; >> cap->codec = BIT(bit); >> cap->domain = VIDC_SESSION_TYPE_ENC; >> >> -- >> 2.34.1 >> >
On 11/6/2024 3:53 PM, Bryan O'Donoghue wrote: > On 06/11/2024 07:25, Vikash Garodia wrote: >>>> cap = &caps[core->codecs_count++]; >>>> cap->codec = BIT(bit); >>>> cap->domain = VIDC_SESSION_TYPE_ENC; >>>> >>> I don't see how codecs_count could be greater than the control, since you >>> increment by one on each loop but >= is fine too I suppose. >> Assume the payload from malicious firmware is packed like below >> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >> ..... >> for 32 or more instances of above type > > But you do this > > cap = &caps[core->codecs_count++]; > > for each bit. Yes. Let say that packet is written more than 32 times in the payload response from bad firmware and each has 1 bit set. core->codecs_count would be incremented beyond the allocated space. Regards, Vikash > > Anyway consider Dmitry's input re only calling this function once instead. > > --- > bod
On Thu, Nov 07, 2024 at 01:47:20PM +0530, Vikash Garodia wrote: > > On 11/5/2024 7:25 PM, Dmitry Baryshkov wrote: > > On Tue, Nov 05, 2024 at 02:24:54PM +0530, Vikash Garodia wrote: > >> There is a possibility that init_codecs is invoked multiple times during > >> manipulated payload from video firmware. In such case, if codecs_count > >> can get incremented to value more than MAX_CODEC_NUM, there can be OOB > >> access. Keep a check for max accessible memory before accessing it. > > > > No. Please make sure that init_codecs() does a correct thing, so that > > core->codecs_count isn't incremented that much (or even better that > > init_codecs() doesn't do anything if it is executed second time). > init_codecs() parses the payload received from firmware and . I don't think we > can control this part when we have something like this from a malicious firmware > payload > HFI_PROPERTY_PARAM_CODEC_SUPPORTED > HFI_PROPERTY_PARAM_CODEC_SUPPORTED > HFI_PROPERTY_PARAM_CODEC_SUPPORTED > ... > Limiting it to second iteration would restrict the functionality when property > HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs. If you can have a malicious firmware (which is owned and signed by Qualcomm / OEM), then you have to be careful and skip duplicates. So instead of just adding new cap to core->caps, you have to go through that array, check that you are not adding a duplicate (and report a [Firmware Bug] for duplicates), check that there is an empty slot, etc. Just ignoring the "extra" entries is not enough. > > Regards, > Vikash > >> > >> 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 | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c > >> index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..27d0172294d5154f4839e8cef172f9a619dfa305 100644 > >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c > >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c > >> @@ -23,6 +23,8 @@ static void init_codecs(struct venus_core *core) > >> return; > >> > >> for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) { > >> + if (core->codecs_count >= MAX_CODEC_NUM) > >> + return; > >> cap = &caps[core->codecs_count++]; > >> cap->codec = BIT(bit); > >> cap->domain = VIDC_SESSION_TYPE_DEC; > >> @@ -30,6 +32,8 @@ static void init_codecs(struct venus_core *core) > >> } > >> > >> for_each_set_bit(bit, &core->enc_codecs, MAX_CODEC_NUM) { > >> + if (core->codecs_count >= MAX_CODEC_NUM) > >> + return; > >> cap = &caps[core->codecs_count++]; > >> cap->codec = BIT(bit); > >> cap->domain = VIDC_SESSION_TYPE_ENC; > >> > >> -- > >> 2.34.1 > >> > >
On 07/11/2024 10:41, Dmitry Baryshkov wrote: >> init_codecs() parses the payload received from firmware and . I don't think we >> can control this part when we have something like this from a malicious firmware >> payload >> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >> ... >> Limiting it to second iteration would restrict the functionality when property >> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs. > If you can have a malicious firmware (which is owned and signed by > Qualcomm / OEM), then you have to be careful and skip duplicates. So > instead of just adding new cap to core->caps, you have to go through > that array, check that you are not adding a duplicate (and report a > [Firmware Bug] for duplicates), check that there is an empty slot, etc. > > Just ignoring the "extra" entries is not enough. +1 This is a more rational argument. If you get a second message, you should surely reinit the whole array i.e. update the array with the new list, as opposed to throwing away the second message because it over-indexes your local storage.. --- bod
On 11/7/2024 5:37 PM, Bryan O'Donoghue wrote: > On 07/11/2024 10:41, Dmitry Baryshkov wrote: >>> init_codecs() parses the payload received from firmware and . I don't think we >>> can control this part when we have something like this from a malicious firmware >>> payload >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >>> ... >>> Limiting it to second iteration would restrict the functionality when property >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs. >> If you can have a malicious firmware (which is owned and signed by >> Qualcomm / OEM), then you have to be careful and skip duplicates. So >> instead of just adding new cap to core->caps, you have to go through >> that array, check that you are not adding a duplicate (and report a >> [Firmware Bug] for duplicates), check that there is an empty slot, etc. >> >> Just ignoring the "extra" entries is not enough. Thinking of something like this for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) { if (core->codecs_count >= MAX_CODEC_NUM) return; cap = &caps[core->codecs_count++]; if (cap->codec == BIT(bit)) --> each code would have unique bitfield return; > +1 > > This is a more rational argument. If you get a second message, you should surely > reinit the whole array i.e. update the array with the new list, as opposed to > throwing away the second message because it over-indexes your local storage.. That would be incorrect to overwrite the array with new list, whenever new payload is received. Regards, Vikash
On Thu, Nov 07, 2024 at 06:32:33PM +0530, Vikash Garodia wrote: > > On 11/7/2024 5:37 PM, Bryan O'Donoghue wrote: > > On 07/11/2024 10:41, Dmitry Baryshkov wrote: > >>> init_codecs() parses the payload received from firmware and . I don't think we > >>> can control this part when we have something like this from a malicious firmware > >>> payload > >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED > >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED > >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED > >>> ... > >>> Limiting it to second iteration would restrict the functionality when property > >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs. > >> If you can have a malicious firmware (which is owned and signed by > >> Qualcomm / OEM), then you have to be careful and skip duplicates. So > >> instead of just adding new cap to core->caps, you have to go through > >> that array, check that you are not adding a duplicate (and report a > >> [Firmware Bug] for duplicates), check that there is an empty slot, etc. > >> > >> Just ignoring the "extra" entries is not enough. > Thinking of something like this > > for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) { > if (core->codecs_count >= MAX_CODEC_NUM) > return; > cap = &caps[core->codecs_count++]; > if (cap->codec == BIT(bit)) --> each code would have unique bitfield > return; This won't work and it's pretty obvious why. > > +1 > > > > This is a more rational argument. If you get a second message, you should surely > > reinit the whole array i.e. update the array with the new list, as opposed to > > throwing away the second message because it over-indexes your local storage.. > That would be incorrect to overwrite the array with new list, whenever new > payload is received. I'd say, don't overwrite the array. Instead the driver should extend it with the new information. > > Regards, > Vikash
On 11/7/2024 6:52 PM, Dmitry Baryshkov wrote: > On Thu, Nov 07, 2024 at 06:32:33PM +0530, Vikash Garodia wrote: >> >> On 11/7/2024 5:37 PM, Bryan O'Donoghue wrote: >>> On 07/11/2024 10:41, Dmitry Baryshkov wrote: >>>>> init_codecs() parses the payload received from firmware and . I don't think we >>>>> can control this part when we have something like this from a malicious firmware >>>>> payload >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED >>>>> ... >>>>> Limiting it to second iteration would restrict the functionality when property >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs. >>>> If you can have a malicious firmware (which is owned and signed by >>>> Qualcomm / OEM), then you have to be careful and skip duplicates. So >>>> instead of just adding new cap to core->caps, you have to go through >>>> that array, check that you are not adding a duplicate (and report a >>>> [Firmware Bug] for duplicates), check that there is an empty slot, etc. >>>> >>>> Just ignoring the "extra" entries is not enough. >> Thinking of something like this >> >> for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) { >> if (core->codecs_count >= MAX_CODEC_NUM) >> return; >> cap = &caps[core->codecs_count++]; >> if (cap->codec == BIT(bit)) --> each code would have unique bitfield >> return; > > This won't work and it's pretty obvious why. Could you please elaborate what would break in above logic ? > >>> +1 >>> >>> This is a more rational argument. If you get a second message, you should surely >>> reinit the whole array i.e. update the array with the new list, as opposed to >>> throwing away the second message because it over-indexes your local storage.. >> That would be incorrect to overwrite the array with new list, whenever new >> payload is received. > > I'd say, don't overwrite the array. Instead the driver should extend it > with the new information. That is exactly the existing patch is currently doing. Regards, Vikash > >> >> Regards, >> Vikash >
On Thu, Nov 07, 2024 at 07:05:15PM +0530, Vikash Garodia wrote: > > On 11/7/2024 6:52 PM, Dmitry Baryshkov wrote: > > On Thu, Nov 07, 2024 at 06:32:33PM +0530, Vikash Garodia wrote: > >> > >> On 11/7/2024 5:37 PM, Bryan O'Donoghue wrote: > >>> On 07/11/2024 10:41, Dmitry Baryshkov wrote: > >>>>> init_codecs() parses the payload received from firmware and . I don't think we > >>>>> can control this part when we have something like this from a malicious firmware > >>>>> payload > >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED > >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED > >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED > >>>>> ... > >>>>> Limiting it to second iteration would restrict the functionality when property > >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs. > >>>> If you can have a malicious firmware (which is owned and signed by > >>>> Qualcomm / OEM), then you have to be careful and skip duplicates. So > >>>> instead of just adding new cap to core->caps, you have to go through > >>>> that array, check that you are not adding a duplicate (and report a > >>>> [Firmware Bug] for duplicates), check that there is an empty slot, etc. > >>>> > >>>> Just ignoring the "extra" entries is not enough. > >> Thinking of something like this > >> > >> for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) { > >> if (core->codecs_count >= MAX_CODEC_NUM) > >> return; > >> cap = &caps[core->codecs_count++]; > >> if (cap->codec == BIT(bit)) --> each code would have unique bitfield > >> return; > > > > This won't work and it's pretty obvious why. > Could you please elaborate what would break in above logic ? After the "cap=&caps[core->codecs_count++]" line 'cap' will point to the new entry, which should not contain valid data. Instead, when processing new 'bit' you should loop over the existing caps and check that there is no match. And only if there is no match the code should be allocating new entry, checking that codecs_count doesn't overflow, etc. > > > > >>> +1 > >>> > >>> This is a more rational argument. If you get a second message, you should surely > >>> reinit the whole array i.e. update the array with the new list, as opposed to > >>> throwing away the second message because it over-indexes your local storage.. > >> That would be incorrect to overwrite the array with new list, whenever new > >> payload is received. > > > > I'd say, don't overwrite the array. Instead the driver should extend it > > with the new information. > That is exactly the existing patch is currently doing. _new_ information, not a copy of the existing information. > > Regards, > Vikash > > > >> > >> Regards, > >> Vikash > >
On 07/11/2024 13:54, Dmitry Baryshkov wrote: >>> I'd say, don't overwrite the array. Instead the driver should extend it >>> with the new information. >> That is exactly the existing patch is currently doing. > _new_ information, not a copy of the existing information. But is this _really_ new information or is it guarding from "malicious" additional messages ? @Vikash is it even a valid use-case for firmware to send one set of capabilities and then send a new set ? It seems to me this should only happen once when the firmware starts up - the firmware won't acquire any new abilities once it has enumerated its set to APSS. So why is it valid to process an additional message at all ? Shouldn't we instead be throwing away redundant updates either silently or with some kind of complaint ? If there's no new data - then this is data we shouldn't bother processing. If it is new data then surely it should be the _current_ and _only_ valid set of data. And if the update is considered "invalid" then why _would_ we accept the update ? I get we're fixing the OOB but I think we should be clear on the validity of the content of the packet. --- bod
On 11/8/2024 5:13 PM, Bryan O'Donoghue wrote: > On 07/11/2024 13:54, Dmitry Baryshkov wrote: >>>> I'd say, don't overwrite the array. Instead the driver should extend it >>>> with the new information. >>> That is exactly the existing patch is currently doing. >> _new_ information, not a copy of the existing information. > > But is this _really_ new information or is it guarding from "malicious" > additional messages ? > > @Vikash is it even a valid use-case for firmware to send one set of capabilities > and then send a new set ? > > It seems to me this should only happen once when the firmware starts up - the > firmware won't acquire any new abilities once it has enumerated its set to APSS. > > So why is it valid to process an additional message at all ? > > Shouldn't we instead be throwing away redundant updates either silently or with > some kind of complaint ? > > If there's no new data - then this is data we shouldn't bother processing. > > If it is new data then surely it should be the _current_ and _only_ valid set of > data. > > And if the update is considered "invalid" then why _would_ we accept the update ? > > I get we're fixing the OOB but I think we should be clear on the validity of the > content of the packet. The payload [1] is all about 2 u32s each for decoder and encoder bit masks, while each bit signifies which codec each supports. So in a good case, it would be always first iteration which would be sufficient. Its a very hypothetical case where a good case would that there are 8 payloads (consider there are 8 supported codecs) with one bit set in each of those 8 payloads. I was initially thinking to cover for this case as well, seems could be a bit of over designing. Maybe set core->codecs_count (to 0) in the beginning of the API should cover the working case. In malicious case, let it continue to override ? Regards, Vikash [1] https://elixir.bootlin.com/linux/v6.12-rc7/source/drivers/media/platform/qcom/venus/hfi_parser.c#L193
diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..27d0172294d5154f4839e8cef172f9a619dfa305 100644 --- a/drivers/media/platform/qcom/venus/hfi_parser.c +++ b/drivers/media/platform/qcom/venus/hfi_parser.c @@ -23,6 +23,8 @@ static void init_codecs(struct venus_core *core) return; for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) { + if (core->codecs_count >= MAX_CODEC_NUM) + return; cap = &caps[core->codecs_count++]; cap->codec = BIT(bit); cap->domain = VIDC_SESSION_TYPE_DEC; @@ -30,6 +32,8 @@ static void init_codecs(struct venus_core *core) } for_each_set_bit(bit, &core->enc_codecs, MAX_CODEC_NUM) { + if (core->codecs_count >= MAX_CODEC_NUM) + return; cap = &caps[core->codecs_count++]; cap->codec = BIT(bit); cap->domain = VIDC_SESSION_TYPE_ENC;
There is a possibility that init_codecs is invoked multiple times during manipulated payload from video firmware. In such case, if codecs_count can get incremented to value more than MAX_CODEC_NUM, there can be OOB access. Keep a check for max accessible memory before accessing it. 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 | 4 ++++ 1 file changed, 4 insertions(+)