linux-kernel - Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add

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

Message-ID: <87pm9377qt.fsf@linux.ibm.com>
Date:   Mon, 20 Mar 2023 10:38:50 -0500
From:   Nathan Lynch <nathanl@...ux.ibm.com>
To:     Markus Elfring <Markus.Elfring@....de>
Cc:     cocci@...ia.fr, LKML <linux-kernel@...r.kernel.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Michael Ellerman <mpe@...erman.id.au>,
        Nicholas Piggin <npiggin@...il.com>,
        Paul Moore <paul@...l-moore.com>,
        linuxppc-dev@...ts.ozlabs.org, kernel-janitors@...r.kernel.org
Subject: Re: powerpc/pseries: Fix exception handling in
 pSeries_reconfig_add_node()

Markus Elfring <Markus.Elfring@....de> writes:
>>>>> The label “out_err” was used to jump to another pointer check despite of
>>>>> the detail in the implementation of the function “pSeries_reconfig_add_node”
>>>>> that it was determined already that the corresponding variable contained
>>>>> a null pointer (because of a failed function call in two cases).
>>>>>
>>>>> 1. Thus return directly after a call of the function “kzalloc” failed.
>>>>>
>>>>> 2. Use more appropriate labels instead.
>>>>>
>>>>> 3. Delete a redundant check.
>>>>>
>>>>> 4. Omit an explicit initialisation for the local variable “err”.
>>>>>
>>>>> This issue was detected by using the Coccinelle software.
>>>> Is there a correctness or safety issue here?
>>> I got the impression that the application of only a single label like “out_err”
>>> resulted in improvable implementation details.
>> I don't understand what you're trying to say here.
>
> What does hinder you to understand the presented change description better
> at the moment?
>
>
>> It doesn't seem to answer my question.
>
>
> I hope that my answer will trigger further helpful considerations.

I don't consider this response constructive, but I want to get this back
on track. It's been brought to my attention that there is in fact a
crash bug in this function's error path:

	np->parent = pseries_of_derive_parent(path);
	if (IS_ERR(np->parent)) {
		err = PTR_ERR(np->parent);
		goto out_err;
	}
...
out_err:
	if (np) {
		of_node_put(np->parent);

np->parent can be an encoded error value, we don't want to of_node_put()
that.

I believe the patch as written happens to fix the issue. Will you please
write it up as a bug fix and resubmit?