[PATCH] s390/raw3270: handle memory allocation failure in 'raw3270_setup_console()'

[PATCH] s390/raw3270: handle memory allocation failure in 'raw3270_setup_console()'

[yskelg]

From: Yunseong Kim <yskelg@gmail.com>

This patch handle potential null pointer dereference in
'raw3270_setup_device()', When 'raw3270_setup_console()' fails to
allocate memory for 'rp' or 'ascebc'.

Signed-off-by: Yunseong Kim <yskelg@gmail.com>
---
 drivers/s390/char/raw3270.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/s390/char/raw3270.c b/drivers/s390/char/raw3270.c
index c57694be9bd3..4e81040eea81 100644
--- a/drivers/s390/char/raw3270.c
+++ b/drivers/s390/char/raw3270.c
@@ -812,7 +812,13 @@ struct raw3270 __init *raw3270_setup_console(void)
 		return ERR_CAST(cdev);
 
 	rp = kzalloc(sizeof(*rp), GFP_KERNEL | GFP_DMA);
+	if (!rp)
+		return ERR_PTR(-ENOMEM);
 	ascebc = kzalloc(256, GFP_KERNEL);
+	if (!ascebc) {
+		kfree(rp);
+		return ERR_PTR(-ENOMEM);
+	}
 	rc = raw3270_setup_device(cdev, rp, ascebc);
 	if (rc)
 		return ERR_PTR(rc);
-- 
2.45.2

Re: [PATCH] s390/raw3270: Handle memory allocation failures in raw3270_setup_console()

[Markus Elfring]

> This patch handle potential null pointer dereference in
> 'raw3270_setup_device()', When 'raw3270_setup_console()' fails to
> allocate memory for 'rp' or 'ascebc'.

1. Can a wording approach (like the following) be a better change description?

   A null pointer is stored in a local variable after a call of
   the function “kzalloc” failed. This pointer was passed to
   a subsequent call of the function “raw3270_setup_device”
   where an undesirable dereference will be performed then.
   Thus add corresponding return value checks.


2. Would you like to add any tags (like “Fixes”) accordingly?


3. The allocated two memory areas are immediately overwritten by the called function.
   Can zero-initialisation be omitted by calling the function “kmalloc” instead?


4. Under which circumstances will development interests grow for increasing
   the application of scope-based resource management?
   https://elixir.bootlin.com/linux/v6.10-rc4/source/include/linux/cleanup.h#L8


Regards,
Markus

Re: [PATCH] s390/raw3270: handle memory allocation failure in 'raw3270_setup_console()'

[Heiko Carstens]

On Sun, Jun 23, 2024 at 09:24:49PM +0900, yskelg@gmail.com wrote:
> From: Yunseong Kim <yskelg@gmail.com>
> 
> This patch handle potential null pointer dereference in
> 'raw3270_setup_device()', When 'raw3270_setup_console()' fails to
> allocate memory for 'rp' or 'ascebc'.
> 
> Signed-off-by: Yunseong Kim <yskelg@gmail.com>
> ---
>  drivers/s390/char/raw3270.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/s390/char/raw3270.c b/drivers/s390/char/raw3270.c
> index c57694be9bd3..4e81040eea81 100644
> --- a/drivers/s390/char/raw3270.c
> +++ b/drivers/s390/char/raw3270.c
> @@ -812,7 +812,13 @@ struct raw3270 __init *raw3270_setup_console(void)
>  		return ERR_CAST(cdev);
>  
>  	rp = kzalloc(sizeof(*rp), GFP_KERNEL | GFP_DMA);
> +	if (!rp)
> +		return ERR_PTR(-ENOMEM);
>  	ascebc = kzalloc(256, GFP_KERNEL);
> +	if (!ascebc) {
> +		kfree(rp);
> +		return ERR_PTR(-ENOMEM);
> +	}
>  	rc = raw3270_setup_device(cdev, rp, ascebc);
>  	if (rc)
>  		return ERR_PTR(rc);

This is kind of pointless since such allocations won't fail.. but
anyway: please make allocation and error handling like it is already
done in raw3270_create_device(); this will also prevent a memory leak
of rp and ascebc in case raw3270_setup_device() fails.