linux-kernel - Re: [RESEND PATCH v1 2/2] misc: fastrpc: detect privileged processes based on group ID

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]

Date:   Thu, 8 Jun 2023 13:36:53 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Ekansh Gupta <quic_ekangupt@...cinc.com>,
        srinivas.kandagatla@...aro.org, linux-arm-msm@...r.kernel.org
Cc:     ekangupt@....qualcomm.com, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, fastrpc.upstream@....qualcomm.com,
        agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org, devicetree@...r.kernel.org
Subject: Re: [RESEND PATCH v1 2/2] misc: fastrpc: detect privileged processes
 based on group ID

On 08/06/2023 12:44, Ekansh Gupta wrote:
> 
> 
> On 6/8/2023 12:17 AM, Krzysztof Kozlowski wrote:
>> On 07/06/2023 18:30, Ekansh Gupta wrote:
>>> Get the information on privileged group IDs during rpmsg probing based
>>> on DT property. Check if the process requesting an offload to remote
>>> subsystem is privileged by comparing it's group ID with privileged
>>> group ID. Initialization process attributes are updated for a
>>> privileged process which is sent to remote process for resource
>>> management.
>>>
>>
>>
>>
>>> +
>>>   static const struct file_operations fastrpc_fops = {
>>>   	.open = fastrpc_device_open,
>>>   	.release = fastrpc_device_release,
>>> @@ -2277,6 +2396,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>   	if (!data)
>>>   		return -ENOMEM;
>>>   
>>> +	err = fastrpc_init_privileged_gids(rdev, "qcom,fastrpc-gids", &data->gidlist);
>>> +	if (err)
>>> +		dev_err(rdev, "Privileged gids init failed.\n");
>>> +
>>
>> What about error paths? No need for cleanup?
>>
> All the necessary clean-up is added as part of 
> fastrpc_init_privileged_gids error path. There is no requirement to have 

Where? How that code is called after fastrpc_device_register() failure?
Or after of_platform_populate() failure?

Please show me the code flow.

> any additional handling in error path other that error log. Also there 
> is no intention to fail the probe in case gid information is not 
> properly read.

This is not related. I don't talk about fastrpc_init_privileged_gids()
failures. Look where did I leave my comment.

Review comments are placed in proper places, not in unrelated parts of
code. The placement of review comment is important as this is the
context of bug in your patch.

Best regards,
Krzysztof