| Message ID | 1691634304-2158-4-git-send-email-quic_vgarodia@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Stanimir Varbanov |
| Headers |
Received: from vger.kernel.org ([23.128.96.18])
by www.linuxtv.org with esmtp (Exim 4.92)
(envelope-from <linux-media-owner@vger.kernel.org>)
id 1qTvNF-00ELC8-A7; Thu, 10 Aug 2023 02:26:07 +0000
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S232296AbjHJC0D (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other);
Wed, 9 Aug 2023 22:26:03 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47894 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S231787AbjHJC0B (ORCPT
<rfc822;linux-media@vger.kernel.org>); Wed, 9 Aug 2023 22:26:01 -0400
Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com
[205.220.180.131])
by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 44AF31BCF;
Wed, 9 Aug 2023 19:26:01 -0700 (PDT)
Received: from pps.filterd (m0279870.ppops.net [127.0.0.1])
by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id
37A2JCqQ010471;
Thu, 10 Aug 2023 02:25:56 GMT
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com;
h=from : to : cc :
subject : date : message-id : in-reply-to : references : mime-version :
content-type; s=qcppdkim1;
bh=Id3qv/5ZcdycMZ8v5o4bZF4k+g39Mbe/s38TYj1GJkI=;
b=iag/iiCz3X8WQ/KoASNhlaMyNTqnMunQxUNIg0xU5ls+avuv9rlKn1bJxTxxV65RMnKr
0cggecLiUekr85WOD9KCbdHCLApX7F4WPwk8tWnbgFtUyb3BRwh7AmeH7EvAiNkeSplW
zwAU5051GVPns+VM4GuSuIPGMcW9pR675gEtRG6uwZJkf4wT7BwjLtdCKI3XyLBly6+h
wdxwMWE3zQmckX+byqpzEAvyC7SFEsLxFytKYVCgepojev3ls3q7ebkIQHvHcc0XB2pX
lhYHeElQrwsY3NyNbM567ai/ziTDnFj+PFSXazzDiDTWMB4TD1Qql/2R4EfbvPDixvln rQ==
Received: from nasanppmta02.qualcomm.com (i-global254.qualcomm.com
[199.106.103.254])
by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3scnsf83v5-1
(version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256
verify=NOT);
Thu, 10 Aug 2023 02:25:56 +0000
Received: from nasanex01a.na.qualcomm.com (nasanex01a.na.qualcomm.com
[10.52.223.231])
by NASANPPMTA02.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id
37A2Ptg6010223
(version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256
verify=NOT);
Thu, 10 Aug 2023 02:25:55 GMT
Received: from hu-vgarodia-hyd.qualcomm.com (10.80.80.8) by
nasanex01a.na.qualcomm.com (10.52.223.231) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
15.2.1118.30; Wed, 9 Aug 2023 19:25:51 -0700
From: Vikash Garodia <quic_vgarodia@quicinc.com>
To: <stanimir.k.varbanov@gmail.com>, <bryan.odonoghue@linaro.org>,
<agross@kernel.org>, <andersson@kernel.org>,
<konrad.dybcio@linaro.org>, <mchehab@kernel.org>,
<hans.verkuil@cisco.com>, <tfiga@chromium.org>
CC: <linux-media@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <stable@vger.kernel.org>,
Vikash Garodia <quic_vgarodia@quicinc.com>
Subject: [PATCH v2 3/4] venus: hfi: add checks to handle capabilities from
firmware
Date: Thu, 10 Aug 2023 07:55:03 +0530
Message-ID: <1691634304-2158-4-git-send-email-quic_vgarodia@quicinc.com>
X-Mailer: git-send-email 2.7.4
In-Reply-To: <1691634304-2158-1-git-send-email-quic_vgarodia@quicinc.com>
References: <1691634304-2158-1-git-send-email-quic_vgarodia@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain
X-Originating-IP: [10.80.80.8]
X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To
nasanex01a.na.qualcomm.com (10.52.223.231)
X-QCInternal: smtphost
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800
signatures=585085
X-Proofpoint-GUID: ilFCVncCkjlMY-_iU9vbPejh-4ZgEk8J
X-Proofpoint-ORIG-GUID: ilFCVncCkjlMY-_iU9vbPejh-4ZgEk8J
X-Proofpoint-Virus-Version: vendor=baseguard
engine=ICAP:2.0.267,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
definitions=2023-08-10_01,2023-08-09_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0
suspectscore=0 bulkscore=0
impostorscore=0 mlxscore=0 spamscore=0 lowpriorityscore=0 adultscore=0
mlxlogscore=999 malwarescore=0 priorityscore=1501 clxscore=1015
phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
engine=8.12.0-2306200000 definitions=main-2308100019
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,
SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-media.vger.kernel.org>
X-Mailing-List: linux-media@vger.kernel.org
X-LSpam-Score: -2.5 (--)
X-LSpam-Report: No,
score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1
autolearn=ham autolearn_force=no
|
| Series |
Venus driver fixes to avoid possible OOB accesses
|
|
Commit Message
Vikash Garodia
Aug. 10, 2023, 2:25 a.m. UTC
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(+)
Comments
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;