linux-kernel - Re: [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAA8EJpoVByj28GFNUKydONsS8UZqO-5eSqSsvZHcEHaJbW+Gig@mail.gmail.com>
Date:   Fri, 3 Feb 2023 17:34:56 +0200
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Mukesh Ojha <quic_mojha@...cinc.com>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()

On Fri, 3 Feb 2023 at 16:53, Mukesh Ojha <quic_mojha@...cinc.com> wrote:
>
>
>
> On 2/3/2023 8:21 PM, Srinivas Kandagatla wrote:
> >
> >
> > On 03/02/2023 10:17, Mukesh Ojha wrote:
> >> Modify qcom_scm_set_download_mode() such that it can support
> >> multiple modes. There is no functional change with this change.
> >>
> >> Signed-off-by: Mukesh Ojha <quic_mojha@...cinc.com>
> >> ---
> >> Changes in v2:
> >>    - Stop changing legacy scm id for dload mode.
> >>
> >>   drivers/firmware/qcom_scm.c | 15 +++++++--------
> >>   include/linux/qcom_scm.h    |  5 +++++
> >>   2 files changed, 12 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> >> index cdbfe54..6245b97 100644
> >> --- a/drivers/firmware/qcom_scm.c
> >> +++ b/drivers/firmware/qcom_scm.c
> >> @@ -400,7 +400,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> >>   }
> >>   EXPORT_SYMBOL(qcom_scm_set_remote_state);
> >> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> >> +static int __qcom_scm_set_dload_mode(struct device *dev, enum
> >> qcom_download_mode mode)
> >>   {
> >>       struct qcom_scm_desc desc = {
> >>           .svc = QCOM_SCM_SVC_BOOT,
> >> @@ -410,12 +410,12 @@ static int __qcom_scm_set_dload_mode(struct
> >> device *dev, bool enable)
> >>           .owner = ARM_SMCCC_OWNER_SIP,
> >>       };
> >> -    desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> >> +    desc.args[1] = mode ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> >
> > I think this line should be:
> >
> >      desc.args[1] = mode;
> >
>
> Should be fine..for backward compatibility as we want to avoid what is
> passed to trust zone without check and since this is legacy code, i
> would like to avoid.

I'd second Srini here. Please remove the ternary operator and pass
mode directly. If you'd like to limit the 'mode' argument, do so
directly (and return an error if the mode is not supported).

However there still exists a bigger problem in my opinion. You are
changing an API. Please provide a user for this API. 'The user will be
provided separately/later/whatever' is usually not enough.



-- 
With best wishes
Dmitry