2023-12-04 02:08:10

by Kunwu Chan

Subject: [PATCH] cxl: Fix null pointer dereference in cxl_get_fd

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure.

Fixes: bdecf76e319a ("cxl: Fix coredump generation when cxl_get_fd() is used")
Signed-off-by: Kunwu Chan <[email protected]>
---
drivers/misc/cxl/api.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
index d85c56530863..bfd7ccd4d7e1 100644
--- a/drivers/misc/cxl/api.c
+++ b/drivers/misc/cxl/api.c
@@ -419,6 +419,10 @@ struct file *cxl_get_fd(struct cxl_context *ctx, struct file_operations *fops,
fops = (struct file_operations *)&afu_fops;

name = kasprintf(GFP_KERNEL, "cxl:%d", ctx->pe);
+ if (!name) {
+ put_unused_fd(fdtmp);
+ return ERR_PTR(-ENOMEM);
+ }
file = cxl_getfile(name, fops, ctx, flags);
kfree(name);
if (IS_ERR(file))
--
2.34.1

2023-12-04 10:43:59

by Frederic Barrat

Subject: Re: [PATCH] cxl: Fix null pointer dereference in cxl_get_fd

On 04/12/2023 03:07, Kunwu Chan wrote:
> kasprintf() returns a pointer to dynamically allocated memory
> which can be NULL upon failure.
>
> Fixes: bdecf76e319a ("cxl: Fix coredump generation when cxl_get_fd() is used")
> Signed-off-by: Kunwu Chan <[email protected]>
> ---
> drivers/misc/cxl/api.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
> index d85c56530863..bfd7ccd4d7e1 100644
> --- a/drivers/misc/cxl/api.c
> +++ b/drivers/misc/cxl/api.c
> @@ -419,6 +419,10 @@ struct file *cxl_get_fd(struct cxl_context *ctx, struct file_operations *fops,
> fops = (struct file_operations *)&afu_fops;
>
> name = kasprintf(GFP_KERNEL, "cxl:%d", ctx->pe);
> + if (!name) {
> + put_unused_fd(fdtmp);
> + return ERR_PTR(-ENOMEM);
> + }


That works, but you might as well follow the existing error path:

name = kasprintf(GFP_KERNEL, "cxl:%d", ctx->pe);
if (!name)
goto err_fd;

Fred


> file = cxl_getfile(name, fops, ctx, flags);
> kfree(name);
> if (IS_ERR(file))

2023-12-05 09:31:05

by Kunwu Chan

Subject: Re: [PATCH] cxl: Fix null pointer dereference in cxl_get_fd

Hi Fred,
Thanks for your reply.


But there is a question, whether we should return an error code in error
path so that the caller of the 'cxl_get_fd' can know the specific
reason. rather than just return NULL.
Such as:
- int rc, flags, fdtmp;
+ int rc = 0, flags, fdtmp;
char *name = NULL;

/* only allow one per context */
- if (ctx->mapping)
- return ERR_PTR(-EEXIST);
+ if (ctx->mapping) {
+ rc = -EEXIST;
+ goto err;
+ }

flags = O_RDWR | O_CLOEXEC;

/* This code is similar to anon_inode_getfd() */
rc = get_unused_fd_flags(flags);
- if (rc < 0)
- return ERR_PTR(rc);
+ if (rc < 0) {
+ goto err;
+ }
fdtmp = rc;



name = kasprintf(GFP_KERNEL, "cxl:%d", ctx->pe);
+ if (!name) {
+ rc = -ENOMEM;
+ goto err_fd;
+ }
file = cxl_getfile(name, fops, ctx, flags);
kfree(name);
@@ -434,6 +437,9 @@ struct file *cxl_get_fd(struct cxl_context *ctx,
struct file_operations *fops,

err_fd:
put_unused_fd(fdtmp);
+err:
+ if (rc)
+ return ERR_PTR(rc);
return NULL;


Thanks again,
Kunwu

On 2023/12/4 18:43, Frederic Barrat wrote:
>
>
> On 04/12/2023 03:07, Kunwu Chan wrote:
>> kasprintf() returns a pointer to dynamically allocated memory
>> which can be NULL upon failure.
>>
>> Fixes: bdecf76e319a ("cxl: Fix coredump generation when cxl_get_fd()
>> is used")
>> Signed-off-by: Kunwu Chan <[email protected]>
>> ---
>>   drivers/misc/cxl/api.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
>> index d85c56530863..bfd7ccd4d7e1 100644
>> --- a/drivers/misc/cxl/api.c
>> +++ b/drivers/misc/cxl/api.c
>> @@ -419,6 +419,10 @@ struct file *cxl_get_fd(struct cxl_context *ctx,
>> struct file_operations *fops,
>>           fops = (struct file_operations *)&afu_fops;
>>       name = kasprintf(GFP_KERNEL, "cxl:%d", ctx->pe);
>> +    if (!name) {
>> +        put_unused_fd(fdtmp);
>> +        return ERR_PTR(-ENOMEM);
>> +    }
>
>
> That works, but you might as well follow the existing error path:
>
>     name = kasprintf(GFP_KERNEL, "cxl:%d", ctx->pe);
>     if (!name)
>         goto err_fd;
>
>   Fred
>
>
>>       file = cxl_getfile(name, fops, ctx, flags);
>>       kfree(name);
>>       if (IS_ERR(file))