2022-02-28 10:51:18

by Hangyu Hua

Subject: [PATCH] powerpc: kernel: fix a refcount leak in format_show()

node needs to be dropped when of_property_read_string fails. So an earlier call
to of_node_put is required here.

Signed-off-by: Hangyu Hua <[email protected]>
---
arch/powerpc/kernel/secvar-sysfs.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
index a0a78aba2083..cd0fa7028d86 100644
--- a/arch/powerpc/kernel/secvar-sysfs.c
+++ b/arch/powerpc/kernel/secvar-sysfs.c
@@ -30,13 +30,12 @@ static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
return -ENODEV;

rc = of_property_read_string(node, "format", &format);
+ of_node_put(node);
if (rc)
return rc;

rc = sprintf(buf, "%s\n", format);

- of_node_put(node);
-
return rc;
}

--
2.25.1

2022-03-01 13:57:28

by Michael Ellerman

Subject: Re: [PATCH] powerpc: kernel: fix a refcount leak in format_show()

Hangyu Hua <[email protected]> writes:
> node needs to be dropped when of_property_read_string fails. So an earlier call
> to of_node_put is required here.

That's true but ...

> diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
> index a0a78aba2083..cd0fa7028d86 100644
> --- a/arch/powerpc/kernel/secvar-sysfs.c
> +++ b/arch/powerpc/kernel/secvar-sysfs.c
> @@ -30,13 +30,12 @@ static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
> return -ENODEV;

There's also a reference leak there ^

So if you're going to touch this code I'd like you to fix both reference
leaks in a single patch please.

Having the error cases set rc and then goto "out" which does the
of_node_put() is the obvious solution I think.

cheers

> rc = of_property_read_string(node, "format", &format);
> + of_node_put(node);
> if (rc)
> return rc;
>
> rc = sprintf(buf, "%s\n", format);
>
> - of_node_put(node);
> -
> return rc;
> }
>
> --
> 2.25.1

2022-03-01 23:47:55

by Tyrel Datwyler

Subject: Re: [PATCH] powerpc: kernel: fix a refcount leak in format_show()

On 3/1/22 04:55, Michael Ellerman wrote:
> Hangyu Hua <[email protected]> writes:
>> node needs to be dropped when of_property_read_string fails. So an earlier call
>> to of_node_put is required here.
>
> That's true but ...
>
>> diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
>> index a0a78aba2083..cd0fa7028d86 100644
>> --- a/arch/powerpc/kernel/secvar-sysfs.c
>> +++ b/arch/powerpc/kernel/secvar-sysfs.c
>> @@ -30,13 +30,12 @@ static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
>> return -ENODEV;
>
> There's also a reference leak there ^
>
> So if you're going to touch this code I'd like you to fix both reference
> leaks in a single patch please.
>
> Having the error cases set rc and then goto "out" which does the
> of_node_put() is the obvious solution I think.

update_kobj_size() in the same source file provides a good example of the
suggested solution.

-Tyrel

>
> cheers
>
>> rc = of_property_read_string(node, "format", &format);
>> + of_node_put(node);
>> if (rc)
>> return rc;
>>
>> rc = sprintf(buf, "%s\n", format);
>>
>> - of_node_put(node);
>> -
>> return rc;
>> }
>>
>> --
>> 2.25.1

2022-03-02 07:28:16

by Hangyu Hua

Subject: Re: [PATCH] powerpc: kernel: fix a refcount leak in format_show()

Thanks. I will resubmit my patch latter.

On 2022/3/2 03:50, Tyrel Datwyler wrote:
> On 3/1/22 04:55, Michael Ellerman wrote:
>> Hangyu Hua <[email protected]> writes:
>>> node needs to be dropped when of_property_read_string fails. So an earlier call
>>> to of_node_put is required here.
>>
>> That's true but ...
>>
>>> diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
>>> index a0a78aba2083..cd0fa7028d86 100644
>>> --- a/arch/powerpc/kernel/secvar-sysfs.c
>>> +++ b/arch/powerpc/kernel/secvar-sysfs.c
>>> @@ -30,13 +30,12 @@ static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
>>> return -ENODEV;
>>
>> There's also a reference leak there ^
>>
>> So if you're going to touch this code I'd like you to fix both reference
>> leaks in a single patch please.
>>
>> Having the error cases set rc and then goto "out" which does the
>> of_node_put() is the obvious solution I think.
>
> update_kobj_size() in the same source file provides a good example of the
> suggested solution.
>
> -Tyrel
>
>>
>> cheers
>>
>>> rc = of_property_read_string(node, "format", &format);
>>> + of_node_put(node);
>>> if (rc)
>>> return rc;
>>>
>>> rc = sprintf(buf, "%s\n", format);
>>>
>>> - of_node_put(node);
>>> -
>>> return rc;
>>> }
>>>
>>> --
>>> 2.25.1
>