linux-kernel - Re: [PATCH v6 3/5] misc: fastrpc: Add support for context Invoke method

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: <32aac00f-39f5-34a7-e1e3-a0eef62daa3f@linaro.org>
Date:   Fri, 8 Feb 2019 14:26:32 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     arnd@...db.de, robh+dt@...nel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, bjorn.andersson@...aro.org,
        bkumar@....qualcomm.com, linux-arm-msm@...r.kernel.org,
        Thierry Escande <thierry.escande@...aro.org>
Subject: Re: [PATCH v6 3/5] misc: fastrpc: Add support for context Invoke
 method



On 08/02/2019 14:11, Greg KH wrote:
>> On 08/02/2019 13:55, Greg KH wrote:
>>>> +
>>>> +		/* Make sure reserved field is set to 0 */
>>>> +		if (args->reserved) {
>>>> +			kfree(args);
>>>> +			return -EINVAL;
>>> Am I wrong in that you are only checking the first reserved field in
>>> this array of args?  Don't you have a whole bunch of them here (nscalars
>>> of them)?  Don't you need to check them all and not just the first one?
>> My Bad, there should be a loop here!
>> I will fix this in next version!
> No, you don't need to loop it here, check it in the call that evaluates
> the individual args, which will catch the issue when people send
> "internal" commands.
> 
Yes, that makes more sense. We can avoid a loop here and do the check in 
  fastrpc_get_args().

> Which reminds me, what happens if a sequence has problems half way
> through the list of args?  Do you "roll back" to the beginning, or just
> abort?  What happens to the internal device state then?

We should abort the invoke if this check fails.

All the arguments are passed in a single invoke call to dsp, so if the 
sanity fails we should not even attempt to send it to dsp.

fastrpc_context_put in fastrpc_internal_invoke() should take care of any 
mappings created half way thru this checks.

--srini