diff mbox series

powerpc/kexec: Fix the return of uninitialized variable

Message ID 20240930075628.125138-1-zhangzekun11@huawei.com (mailing list archive)
State Accepted
Commit 83b5a407fbb73e6965adfb4bd0a803724bf87f96
Headers show
Series powerpc/kexec: Fix the return of uninitialized variable | expand

Checks

Context Check Description
snowpatch_ozlabs/github-powerpc_ppctests success Successfully ran 8 jobs.
snowpatch_ozlabs/github-powerpc_selftests success Successfully ran 8 jobs.
snowpatch_ozlabs/github-powerpc_sparse success Successfully ran 4 jobs.
snowpatch_ozlabs/github-powerpc_clang success Successfully ran 5 jobs.
snowpatch_ozlabs/github-powerpc_kernel_qemu success Successfully ran 21 jobs.

Commit Message

Zhang Zekun Sept. 30, 2024, 7:56 a.m. UTC
The of_property_read_u64() can fail and remain the variable uninitialized,
which will then be used. Return error if we failed to read the property.

Fixes: 2e6bd221d96f ("powerpc/kexec_file: Enable early kernel OPAL calls")
Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 arch/powerpc/kexec/file_load_64.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Christophe Leroy Sept. 30, 2024, 8:27 a.m. UTC | #1
Le 30/09/2024 à 09:56, Zhang Zekun a écrit :
> [Vous ne recevez pas souvent de courriers de zhangzekun11@huawei.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> The of_property_read_u64() can fail and remain the variable uninitialized,

Replace "remain" by "leave".

> which will then be used. Return error if we failed to read the property.

Rewrite to avoid "we".  For instance "Return error if reading the 
property failed"

> 
> Fixes: 2e6bd221d96f ("powerpc/kexec_file: Enable early kernel OPAL calls")
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---
>   arch/powerpc/kexec/file_load_64.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
> index 9738adabeb1f..dc65c1391157 100644
> --- a/arch/powerpc/kexec/file_load_64.c
> +++ b/arch/powerpc/kexec/file_load_64.c
> @@ -736,13 +736,18 @@ int setup_purgatory_ppc64(struct kimage *image, const void *slave_code,
>          if (dn) {
>                  u64 val;
> 
> -               of_property_read_u64(dn, "opal-base-address", &val);
> +               ret = of_property_read_u64(dn, "opal-base-address", &val);
> +               if (ret)
> +                       goto out;
> +
>                  ret = kexec_purgatory_get_set_symbol(image, "opal_base", &val,
>                                                       sizeof(val), false);
>                  if (ret)
>                          goto out;
> 
> -               of_property_read_u64(dn, "opal-entry-address", &val);
> +               ret = of_property_read_u64(dn, "opal-entry-address", &val);
> +               if (ret)
> +                       goto out;
>                  ret = kexec_purgatory_get_set_symbol(image, "opal_entry", &val,
>                                                       sizeof(val), false);
>          }
> --
> 2.17.1
>
Madhavan Srinivasan Sept. 30, 2024, 9:01 a.m. UTC | #2
On 9/30/24 1:57 PM, Christophe Leroy wrote:
> 
> 
> Le 30/09/2024 à 09:56, Zhang Zekun a écrit :
>> [Vous ne recevez pas souvent de courriers de zhangzekun11@huawei.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>
>> The of_property_read_u64() can fail and remain the variable uninitialized,
> 
> Replace "remain" by "leave".
> 
>> which will then be used. Return error if we failed to read the property.
> 
> Rewrite to avoid "we".  For instance "Return error if reading the property failed"
> 
>>
>> Fixes: 2e6bd221d96f ("powerpc/kexec_file: Enable early kernel OPAL calls")
>> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
>> ---
>>   arch/powerpc/kexec/file_load_64.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
>> index 9738adabeb1f..dc65c1391157 100644
>> --- a/arch/powerpc/kexec/file_load_64.c
>> +++ b/arch/powerpc/kexec/file_load_64.c
>> @@ -736,13 +736,18 @@ int setup_purgatory_ppc64(struct kimage *image, const void *slave_code,
>>          if (dn) {
>>                  u64 val;

Instead cant we init val as -1 ??
Why to add check?


>>
>> -               of_property_read_u64(dn, "opal-base-address", &val);
>> +               ret = of_property_read_u64(dn, "opal-base-address", &val);
>> +               if (ret)
>> +                       goto out;
>> +
>>                  ret = kexec_purgatory_get_set_symbol(image, "opal_base", &val,
>>                                                       sizeof(val), false);
>>                  if (ret)
>>                          goto out;
>>
>> -               of_property_read_u64(dn, "opal-entry-address", &val);
>> +               ret = of_property_read_u64(dn, "opal-entry-address", &val);
>> +               if (ret)
>> +                       goto out;
>>                  ret = kexec_purgatory_get_set_symbol(image, "opal_entry", &val,
>>                                                       sizeof(val), false);
>>          }
>> -- 
>> 2.17.1
>>
Zhang Zekun Oct. 8, 2024, 8:53 a.m. UTC | #3
在 2024/9/30 17:01, Madhavan Srinivasan 写道:
> 
> 
> On 9/30/24 1:57 PM, Christophe Leroy wrote:
>>
>>
>> Le 30/09/2024 à 09:56, Zhang Zekun a écrit :
>>> [Vous ne recevez pas souvent de courriers de zhangzekun11@huawei.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> The of_property_read_u64() can fail and remain the variable uninitialized,
>>
>> Replace "remain" by "leave".
>>
>>> which will then be used. Return error if we failed to read the property.
>>
>> Rewrite to avoid "we".  For instance "Return error if reading the property failed"
>>
>>>
>>> Fixes: 2e6bd221d96f ("powerpc/kexec_file: Enable early kernel OPAL calls")
>>> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
>>> ---
>>>    arch/powerpc/kexec/file_load_64.c | 9 +++++++--
>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
>>> index 9738adabeb1f..dc65c1391157 100644
>>> --- a/arch/powerpc/kexec/file_load_64.c
>>> +++ b/arch/powerpc/kexec/file_load_64.c
>>> @@ -736,13 +736,18 @@ int setup_purgatory_ppc64(struct kimage *image, const void *slave_code,
>>>           if (dn) {
>>>                   u64 val;
> 
> Instead cant we init val as -1 ??
> Why to add check?
> 

Hi, Madhavan,

I am not sure when the default value -1 will be checked, and it would be 
more obvious to me to add check when reading property failed. Besides, 
in arch/powerpc, checking the return val when of_property_read_u64() 
failed seems to be a more common way.

Best Regards,
Zekun
Michael Ellerman Nov. 17, 2024, 12:09 p.m. UTC | #4
On Mon, 30 Sep 2024 15:56:28 +0800, Zhang Zekun wrote:
> The of_property_read_u64() can fail and remain the variable uninitialized,
> which will then be used. Return error if we failed to read the property.
> 
> 

Applied to powerpc/next.

[1/1] powerpc/kexec: Fix the return of uninitialized variable
      https://git.kernel.org/powerpc/c/83b5a407fbb73e6965adfb4bd0a803724bf87f96

cheers
diff mbox series

Patch

diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
index 9738adabeb1f..dc65c1391157 100644
--- a/arch/powerpc/kexec/file_load_64.c
+++ b/arch/powerpc/kexec/file_load_64.c
@@ -736,13 +736,18 @@  int setup_purgatory_ppc64(struct kimage *image, const void *slave_code,
 	if (dn) {
 		u64 val;
 
-		of_property_read_u64(dn, "opal-base-address", &val);
+		ret = of_property_read_u64(dn, "opal-base-address", &val);
+		if (ret)
+			goto out;
+
 		ret = kexec_purgatory_get_set_symbol(image, "opal_base", &val,
 						     sizeof(val), false);
 		if (ret)
 			goto out;
 
-		of_property_read_u64(dn, "opal-entry-address", &val);
+		ret = of_property_read_u64(dn, "opal-entry-address", &val);
+		if (ret)
+			goto out;
 		ret = kexec_purgatory_get_set_symbol(image, "opal_entry", &val,
 						     sizeof(val), false);
 	}