linux-hardening - Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

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]

Date:   Mon, 8 May 2023 12:40:45 +0530
From:   Mukesh Ojha <quic_mojha@...cinc.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        <agross@...nel.org>, <andersson@...nel.org>,
        <konrad.dybcio@...aro.org>, <corbet@....net>,
        <keescook@...omium.org>, <tony.luck@...el.com>,
        <gpiccoli@...lia.com>, <catalin.marinas@....com>,
        <will@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <robh+dt@...nel.org>, <linus.walleij@...aro.org>,
        <linux-gpio@...r.kernel.org>, <srinivas.kandagatla@...aro.org>
CC:     <linux-arm-msm@...r.kernel.org>,
        <linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-hardening@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver



On 5/4/2023 10:04 PM, Krzysztof Kozlowski wrote:
> On 04/05/2023 17:21, Krzysztof Kozlowski wrote:
>>>>
>>>>> +	ret = qcom_minidump_init_apss_subsystem(md);
>>>>> +	if (ret) {
>>>>> +		dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
>>>>> +		goto unlock;
>>>>> +	}
>>>>> +
>>>>> +	__md = md;
>>>>
>>>> No. This is a platform device, so it can have multiple instances.
>>>
>>> It can have only one instance that is created from SMEM driver probe.
>>
>> Anyone can instantiate more of them.... how did you solve it?
> 
> To clarify - sprinkling more of singletons makes everything tightly
> coupled, difficult to debug and non-portable. You cannot have two
> instances, you have to control concurrent initialization by yourself in
> each of such singletons.
> 
> I understand sometimes they are unavoidable, for example when this does
> not map to hardware property. However here you have the parent - smem -
> which can return you valid instance. Thus you avoid entire problem of
> file-scope variables.

I get your point, why one's should avoid file scope variables.


This is infrastructure driver and will not have multiple instances and 
even if it happens could be avoided with with the help of global mutex 
and protect below function which i am already doing at the moment and 
fail the other probe if it is already initialized with proper logging..e.g

"already initialized..."


ret = qcom_minidump_init_apss_subsystem(md);


And this will be in-lined with

/* Pointer to the one and only smem handle */
static struct qcom_smem *__smem;

Let me know if you still disagree...and have some other way ?


-- Mukesh


> 
> Best regards,
> Krzysztof
>