2022-01-05 06:57:20

by Jiasheng Jiang

Subject: [PATCH] thermal/int340x_thermal: Check for null pointer after calling kmemdup

As the possible failure of the allocation, kmemdup() may return NULL
pointer.
Therefore, it should be better to check the return value of kmemdup().
If fails, just free 'buffer.pointer' and directly return is enough, same
as the way that 'obj' fails above.

Fixes: 0ba13c763aac ("thermal/int340x_thermal: Export GDDV")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
index 823354a1a91a..999b5682c28a 100644
--- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
@@ -462,6 +462,11 @@ static void int3400_setup_gddv(struct int3400_thermal_priv *priv)
priv->data_vault = kmemdup(obj->package.elements[0].buffer.pointer,
obj->package.elements[0].buffer.length,
GFP_KERNEL);
+ if (!priv->data_vault) {
+ kfree(buffer.pointer);
+ return;
+ }
+
bin_attr_data_vault.private = priv->data_vault;
bin_attr_data_vault.size = obj->package.elements[0].buffer.length;
kfree(buffer.pointer);
--
2.25.1

2022-01-07 12:05:29

by Daniel Lezcano

Subject: Re: [PATCH] thermal/int340x_thermal: Check for null pointer after calling kmemdup

On 05/01/2022 07:56, Jiasheng Jiang wrote:
> As the possible failure of the allocation, kmemdup() may return NULL
> pointer.
> Therefore, it should be better to check the return value of kmemdup().
> If fails, just free 'buffer.pointer' and directly return is enough, same
> as the way that 'obj' fails above.
>
> Fixes: 0ba13c763aac ("thermal/int340x_thermal: Export GDDV")
> Signed-off-by: Jiasheng Jiang <[email protected]>
> ---
> drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index 823354a1a91a..999b5682c28a 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -462,6 +462,11 @@ static void int3400_setup_gddv(struct int3400_thermal_priv *priv)
> priv->data_vault = kmemdup(obj->package.elements[0].buffer.pointer,
> obj->package.elements[0].buffer.length,
> GFP_KERNEL);
> + if (!priv->data_vault) {
> + kfree(buffer.pointer);
> + return;
> + }
> +

There is another kfree on error before

Please replace those by a goto out_kfree;

> bin_attr_data_vault.private = priv->data_vault;
> bin_attr_data_vault.size = obj->package.elements[0].buffer.length;

out_kfree;
> kfree(buffer.pointer);
>

Why there is no error code returned to the caller?

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2022-01-07 14:33:01

by Jiasheng Jiang

Subject: Re: Re: [PATCH] thermal/int340x_thermal: Check for null pointer after calling kmemdup

On Fri, Jan 07, 2022 at 08:05:17PM +0800, Daniel Lezcano wrote:
>> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
>> index 823354a1a91a..999b5682c28a 100644
>> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
>> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
>> @@ -462,6 +462,11 @@ static void int3400_setup_gddv(struct int3400_thermal_priv *priv)
>> priv->data_vault = kmemdup(obj->package.elements[0].buffer.pointer,
>> obj->package.elements[0].buffer.length,
>> GFP_KERNEL);
>> + if (!priv->data_vault) {
>> + kfree(buffer.pointer);
>> + return;
>> + }
>> +
>
> There is another kfree on error before
>
> Please replace those by a goto out_kfree;
>
>> bin_attr_data_vault.private = priv->data_vault;
>> bin_attr_data_vault.size = obj->package.elements[0].buffer.length;
>
> out_kfree;
>> kfree(buffer.pointer);
>>

Ok, I will submit new patch to replace those.

> Why there is no error code returned to the caller?

Well, I check the commit 0ba13c763aac ("thermal/int340x_thermal: Export GDDV")
and find that it was designed to return without error.
And it seems that the 'bin_attr_data_vault.size' is related to the
'bin_attr_data_vault.private'.
If the size is 0, then the array will not be used.
Therefore, I think it is unnecessary to return error.

Sincerely thanks,
Jiang