2023-06-14 10:21:30

by Jiasheng Jiang

Subject: [PATCH] pstore/platform: Add check for kstrdup

Add check for the return value of kstrdup() and return the error
if it fails in order to avoid NULL pointer dereference.

Fixes: 563ca40ddf40 ("pstore/platform: Switch pstore_info::name to const")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
fs/pstore/platform.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cbc0b468c1ab..afe07f0d1216 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -631,6 +631,10 @@ int pstore_register(struct pstore_info *psi)
* through /sys/module/pstore/parameters/backend
*/
backend = kstrdup(psi->name, GFP_KERNEL);
+ if (!backend) {
+ mutex_unlock(&psinfo_lock);
+ return -ENOMEM;
+ }

pr_info("Registered %s as persistent store backend\n", psi->name);

--
2.25.1

2023-06-14 19:14:18

by Kees Cook

Subject: Re: [PATCH] pstore/platform: Add check for kstrdup

On Wed, Jun 14, 2023 at 06:00:20PM +0800, Jiasheng Jiang wrote:
> Add check for the return value of kstrdup() and return the error
> if it fails in order to avoid NULL pointer dereference.
>
> Fixes: 563ca40ddf40 ("pstore/platform: Switch pstore_info::name to const")
> Signed-off-by: Jiasheng Jiang <[email protected]>
> ---
> fs/pstore/platform.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index cbc0b468c1ab..afe07f0d1216 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -631,6 +631,10 @@ int pstore_register(struct pstore_info *psi)
> * through /sys/module/pstore/parameters/backend
> */
> backend = kstrdup(psi->name, GFP_KERNEL);
> + if (!backend) {
> + mutex_unlock(&psinfo_lock);
> + return -ENOMEM;
> + }

Hmm, I think this isn't the right place since there's been a bunch of
other allocations and registrations. I think it would be better to
allocate a copy (but not assign to "backend" yet) earlier, perhaps
before the taking the psinfo_lock lock? Like:

char *new_backend;
...
new_backend = kstrdup(psi->name, GFP_KERNEL);
if (!new_backend)
return -ENOMEM;

mutex_lock(&psinfo_lock);
if (psinfo) {
...
mutex_unlock(&psinfo_lock);
kfree(new_backend);
return -EBUSY;
}
...
backend = new_backend;


-Kees

--
Kees Cook

2023-06-15 03:31:49

by Jiasheng Jiang

Subject: Re: [PATCH] pstore/platform: Add check for kstrdup

On Thu, Jun 15, 2023 at 03:01:30PM +0800, Kees Cook wrote:
> On Wed, Jun 14, 2023 at 06:00:20PM +0800, Jiasheng Jiang wrote:
>> Add check for the return value of kstrdup() and return the error
>> if it fails in order to avoid NULL pointer dereference.
>>
>> Fixes: 563ca40ddf40 ("pstore/platform: Switch pstore_info::name to const")
>> Signed-off-by: Jiasheng Jiang <[email protected]>
>> ---
>> fs/pstore/platform.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index cbc0b468c1ab..afe07f0d1216 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -631,6 +631,10 @@ int pstore_register(struct pstore_info *psi)
>> * through /sys/module/pstore/parameters/backend
>> */
>> backend = kstrdup(psi->name, GFP_KERNEL);
>> + if (!backend) {
>> + mutex_unlock(&psinfo_lock);
>> + return -ENOMEM;
>> + }
>
> Hmm, I think this isn't the right place since there's been a bunch of
> other allocations and registrations. I think it would be better to
> allocate a copy (but not assign to "backend" yet) earlier, perhaps
> before the taking the psinfo_lock lock? Like:
>
> char *new_backend;
> ...
> new_backend = kstrdup(psi->name, GFP_KERNEL);
> if (!new_backend)
> return -ENOMEM;
>
> mutex_lock(&psinfo_lock);
> if (psinfo) {
> ...
> mutex_unlock(&psinfo_lock);
> kfree(new_backend);
> return -EBUSY;
> }
> ...
> backend = new_backend;

Fine, I will submit a v2 to allocate earlier.

Thanks,
Jiasheng