2023-11-22 09:06:16

by Kunwu Chan

Subject: [PATCH] powerpc/mm: Fix null-pointer dereference in pgtable_cache_add

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure. Ensure the allocation was successful
by checking the pointer validity.

Signed-off-by: Kunwu Chan <[email protected]>
---
arch/powerpc/mm/init-common.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
index 119ef491f797..0884fc601c46 100644
--- a/arch/powerpc/mm/init-common.c
+++ b/arch/powerpc/mm/init-common.c
@@ -139,6 +139,8 @@ void pgtable_cache_add(unsigned int shift)

align = max_t(unsigned long, align, minalign);
name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift);
+ if (!name)
+ return;
new = kmem_cache_create(name, table_size, align, 0, ctor(shift));
if (!new)
panic("Could not allocate pgtable cache for order %d", shift);
--
2.34.1

2023-11-24 15:17:46

by Christophe Leroy

Subject: Re: [PATCH] powerpc/mm: Fix null-pointer dereference in pgtable_cache_add

Le 22/11/2023 à 10:00, Kunwu Chan a écrit :
> [Vous ne recevez pas souvent de courriers de [email protected]. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>
> kasprintf() returns a pointer to dynamically allocated memory
> which can be NULL upon failure. Ensure the allocation was successful
> by checking the pointer validity.

Are you sure this is needed ? Did you check what happens what name is NULL ?

If I followed stuff correctly, I end up in function
sysfs_do_create_link_sd() which already handles the NULL name case which
a big hammer warning.

>
> Signed-off-by: Kunwu Chan <[email protected]>
> ---
> arch/powerpc/mm/init-common.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
> index 119ef491f797..0884fc601c46 100644
> --- a/arch/powerpc/mm/init-common.c
> +++ b/arch/powerpc/mm/init-common.c
> @@ -139,6 +139,8 @@ void pgtable_cache_add(unsigned int shift)
>
> align = max_t(unsigned long, align, minalign);
> name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift);
> + if (!name)
> + return;
> new = kmem_cache_create(name, table_size, align, 0, ctor(shift));
> if (!new)
> panic("Could not allocate pgtable cache for order %d", shift);
> --
> 2.34.1
>

2023-11-27 06:05:27

by Kunwu Chan

Subject: Re: [PATCH] powerpc/mm: Fix null-pointer dereference in pgtable_cache_add

Hi Christophe,

Thanks for your reply.
It's my bad. According your reply, i read the code in
sysfs_do_create_link_sd.There is a null pointer check indeed.

My intention was to check null pointer after memory allocation.
Whether we can add a comment here for someone like me, the null pointer
check is no need here?

Thanks,
Kunwu

On 2023/11/24 23:17, Christophe Leroy wrote:
>
>
> Le 22/11/2023 à 10:00, Kunwu Chan a écrit :
>> [Vous ne recevez pas souvent de courriers de [email protected]. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>
>> kasprintf() returns a pointer to dynamically allocated memory
>> which can be NULL upon failure. Ensure the allocation was successful
>> by checking the pointer validity.
>
> Are you sure this is needed ? Did you check what happens what name is NULL ?
>
> If I followed stuff correctly, I end up in function
> sysfs_do_create_link_sd() which already handles the NULL name case which
> a big hammer warning.
>
>>
>> Signed-off-by: Kunwu Chan <[email protected]>
>> ---
>> arch/powerpc/mm/init-common.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
>> index 119ef491f797..0884fc601c46 100644
>> --- a/arch/powerpc/mm/init-common.c
>> +++ b/arch/powerpc/mm/init-common.c
>> @@ -139,6 +139,8 @@ void pgtable_cache_add(unsigned int shift)
>>
>> align = max_t(unsigned long, align, minalign);
>> name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift);
>> + if (!name)
>> + return;
>> new = kmem_cache_create(name, table_size, align, 0, ctor(shift));
>> if (!new)
>> panic("Could not allocate pgtable cache for order %d", shift);
>> --
>> 2.34.1
>>

2023-11-28 11:32:52

by Michael Ellerman

Subject: Re: [PATCH] powerpc/mm: Fix null-pointer dereference in pgtable_cache_add

Kunwu Chan <[email protected]> writes:
> Hi Christophe,
>
> Thanks for your reply.
> It's my bad. According your reply, i read the code in
> sysfs_do_create_link_sd.There is a null pointer check indeed.
>
> My intention was to check null pointer after memory allocation.
> Whether we can add a comment here for someone like me, the null pointer
> check is no need here?

I don't mind there being a NULL check for name.

But the code shouldn't silently return if name can't be allocated.
Notice that if we can't create the cache we *panic*. A failure to
allocate name, which causes us to skip the cache creation, needs to also
panic.

cheers

> On 2023/11/24 23:17, Christophe Leroy wrote:
>>
>>
>> Le 22/11/2023 à 10:00, Kunwu Chan a écrit :
>>> [Vous ne recevez pas souvent de courriers de [email protected]. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> kasprintf() returns a pointer to dynamically allocated memory
>>> which can be NULL upon failure. Ensure the allocation was successful
>>> by checking the pointer validity.
>>
>> Are you sure this is needed ? Did you check what happens what name is NULL ?
>>
>> If I followed stuff correctly, I end up in function
>> sysfs_do_create_link_sd() which already handles the NULL name case which
>> a big hammer warning.
>>
>>>
>>> Signed-off-by: Kunwu Chan <[email protected]>
>>> ---
>>> arch/powerpc/mm/init-common.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
>>> index 119ef491f797..0884fc601c46 100644
>>> --- a/arch/powerpc/mm/init-common.c
>>> +++ b/arch/powerpc/mm/init-common.c
>>> @@ -139,6 +139,8 @@ void pgtable_cache_add(unsigned int shift)
>>>
>>> align = max_t(unsigned long, align, minalign);
>>> name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift);
>>> + if (!name)
>>> + return;
>>> new = kmem_cache_create(name, table_size, align, 0, ctor(shift));
>>> if (!new)
>>> panic("Could not allocate pgtable cache for order %d", shift);
>>> --
>>> 2.34.1
>>>

2023-11-30 08:46:29

by Kunwu Chan

Subject: Re: [PATCH] powerpc/mm: Fix null-pointer dereference in pgtable_cache_add

Thanks for your reply.

Ok, I know what you mean, when name is NULL. The process should be
aborted and the specific reason for the error should be printed, not
just return.

I will update v2 patch with "panic".

Thanks again,
Kunwu

On 2023/11/28 19:32, Michael Ellerman wrote:
> Kunwu Chan <[email protected]> writes:
>> Hi Christophe,
>>
>> Thanks for your reply.
>> It's my bad. According your reply, i read the code in
>> sysfs_do_create_link_sd.There is a null pointer check indeed.
>>
>> My intention was to check null pointer after memory allocation.
>> Whether we can add a comment here for someone like me, the null pointer
>> check is no need here?
>
> I don't mind there being a NULL check for name.
>
> But the code shouldn't silently return if name can't be allocated.
> Notice that if we can't create the cache we *panic*. A failure to
> allocate name, which causes us to skip the cache creation, needs to also
> panic.
>
> cheers
>
>> On 2023/11/24 23:17, Christophe Leroy wrote:
>>>
>>>
>>> Le 22/11/2023 à 10:00, Kunwu Chan a écrit :
>>>> [Vous ne recevez pas souvent de courriers de [email protected]. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>>>
>>>> kasprintf() returns a pointer to dynamically allocated memory
>>>> which can be NULL upon failure. Ensure the allocation was successful
>>>> by checking the pointer validity.
>>>
>>> Are you sure this is needed ? Did you check what happens what name is NULL ?
>>>
>>> If I followed stuff correctly, I end up in function
>>> sysfs_do_create_link_sd() which already handles the NULL name case which
>>> a big hammer warning.
>>>
>>>>
>>>> Signed-off-by: Kunwu Chan <[email protected]>
>>>> ---
>>>> arch/powerpc/mm/init-common.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
>>>> index 119ef491f797..0884fc601c46 100644
>>>> --- a/arch/powerpc/mm/init-common.c
>>>> +++ b/arch/powerpc/mm/init-common.c
>>>> @@ -139,6 +139,8 @@ void pgtable_cache_add(unsigned int shift)
>>>>
>>>> align = max_t(unsigned long, align, minalign);
>>>> name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift);
>>>> + if (!name)
>>>> + return;
>>>> new = kmem_cache_create(name, table_size, align, 0, ctor(shift));
>>>> if (!new)
>>>> panic("Could not allocate pgtable cache for order %d", shift);
>>>> --
>>>> 2.34.1
>>>>