linux-kernel - Re: [PATCH v3 08/10] drivers: qcom: rpmh-rsc: Don't double-check rpmh

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] [day] [month] [year] [list]

Message-ID: <CAD=FV=XQbtrkK1u6jTuc-pP4fUx0av3eMYznSkUSC2io7BGmmg@mail.gmail.com>
Date:   Wed, 8 Apr 2020 17:10:01 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        Lina Iyer <ilina@...eaurora.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Evan Green <evgreen@...omium.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 08/10] drivers: qcom: rpmh-rsc: Don't double-check rpmh

Hi,

On Wed, Apr 8, 2020 at 5:24 AM Maulik Shah <mkshah@...eaurora.org> wrote:
>
> Hi,
>
> In rpmh.c, rpmh_write_async() and rpmh_write_batch() uses
> __fill_rpmh_msg() which already checks for below payload conditions.
>
> so i am ok to remove duplicate checks from rpmh-rsc.c
>
> can you please add payload at the end of subject.
>
> drivers: qcom: rpmh-rsc: Don't double-check rpmh payload
>
> Other than this.
>
> Reviewed-by: Maulik Shah <mkshah@...eaurora.org>
> Tested-by: Maulik Shah <mkshah@...eaurora.org>

Thanks!  Bjorn / Andy: if you want me to spin my series I'm happy to.
I'm also happy to just let you fix this nit in the commit message and
the other one Maulik had when applying.  Just let me know.


> Note:
>
> rpmh_write() is not using __fill_rpmh_msg() and have replica as below,
> probably since it was declares message on stack instead of using malloc()
>
>          if (!cmd || !n || n > MAX_RPMH_PAYLOAD)
>                  return -EINVAL;
>
>          memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd));
>          rpm_msg.msg.num_cmds = n;
>
> Making a note to remove above if check and start using __fill_rpmh_msg()
> here as well to do memcpy() and num_cmds initilization.
>
> Although it may end up writing msg.state and msg.cmd twice (once during
> defining msg on stack and then during fill rpmh msg) but it should be ok.
>
> Below two lines from __rpmh_write() can be removed as well.
>
>          rpm_msg->msg.state = state;
>
> DEFINE_RPMH_MSG_ONSTACK() and __fill_rpmh_msg() seems taking care of
> initializing msg.state already, so we should be good.
>
> if you are spinning a new version and want to include above change as
> well, i am ok.
>
> if not, i can push separate patch to update this as well once my series
> to invoke rpmh_flush() gets picked up.

I'd sorta be inclined to wait and do this later just because it seems
like we've got enough changes stacked together right now...

-Doug