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

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

[yskelg]

From: Yunseong Kim <yskelg@gmail.com>

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.
The allocated each memory areas are immediately overwritten by the called
function zero-initialisation be omitted by calling the "kmalloc" instead.
After "ccw_device_enable_console" succeeds, set the bit raw3270 flag to
RAW3270_FLAGS_CONSOLE.

Fixes: 33403dcfcdfd ("[S390] 3270 console: convert from bootmem to slab")
Cc: linux-s390@vger.kernel.org
Signed-off-by: Yunseong Kim <yskelg@gmail.com>
---
 drivers/s390/char/raw3270.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/char/raw3270.c b/drivers/s390/char/raw3270.c
index c57694be9bd3..4f3f98bcbc83 100644
--- a/drivers/s390/char/raw3270.c
+++ b/drivers/s390/char/raw3270.c
@@ -811,18 +811,28 @@ struct raw3270 __init *raw3270_setup_console(void)
 	if (IS_ERR(cdev))
 		return ERR_CAST(cdev);
 
-	rp = kzalloc(sizeof(*rp), GFP_KERNEL | GFP_DMA);
-	ascebc = kzalloc(256, GFP_KERNEL);
+	rp = kmalloc(sizeof(*rp), GFP_KERNEL | GFP_DMA);
+	if (!rp)
+		return ERR_PTR(-ENOMEM);
+	ascebc = kmalloc(256, GFP_KERNEL);
+	if (!ascebc) {
+		kfree(rp);
+		return ERR_PTR(-ENOMEM);
+	}
 	rc = raw3270_setup_device(cdev, rp, ascebc);
-	if (rc)
+	if (rc) {
+		kfree(ascebc);
+		kfree(rp);
 		return ERR_PTR(rc);
-	set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
-
+	}
 	rc = ccw_device_enable_console(cdev);
 	if (rc) {
 		ccw_device_destroy_console(cdev);
+		kfree(ascebc);
+		kfree(rp);
 		return ERR_PTR(rc);
 	}
+	set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
 
 	spin_lock_irqsave(get_ccwdev_lock(rp->cdev), flags);
 	do {
-- 
2.45.2

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

[Heiko Carstens]

On Tue, Jun 25, 2024 at 10:32:26AM +0900, yskelg@gmail.com wrote:
> From: Yunseong Kim <yskelg@gmail.com>
> 
> 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.
> The allocated each memory areas are immediately overwritten by the called
> function zero-initialisation be omitted by calling the "kmalloc" instead.
> After "ccw_device_enable_console" succeeds, set the bit raw3270 flag to
> RAW3270_FLAGS_CONSOLE.
> 
> Fixes: 33403dcfcdfd ("[S390] 3270 console: convert from bootmem to slab")
> Cc: linux-s390@vger.kernel.org
> Signed-off-by: Yunseong Kim <yskelg@gmail.com>
> ---
>  drivers/s390/char/raw3270.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
...
>  	rc = raw3270_setup_device(cdev, rp, ascebc);
> -	if (rc)
> +	if (rc) {
> +		kfree(ascebc);
> +		kfree(rp);
>  		return ERR_PTR(rc);
> -	set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
> -
> +	}
>  	rc = ccw_device_enable_console(cdev);
>  	if (rc) {
>  		ccw_device_destroy_console(cdev);
> +		kfree(ascebc);
> +		kfree(rp);
>  		return ERR_PTR(rc);
>  	}
> +	set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);

Why did you move the set_bit() call?

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

[Yunseong Kim]

Hi Heiko,

On 6/25/24 3:31 오후, Heiko Carstens wrote:
> On Tue, Jun 25, 2024 at 10:32:26AM +0900, yskelg@gmail.com wrote:
>> From: Yunseong Kim <yskelg@gmail.com>
>>
>> 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.
>> The allocated each memory areas are immediately overwritten by the called
>> function zero-initialisation be omitted by calling the "kmalloc" instead.
>> After "ccw_device_enable_console" succeeds, set the bit raw3270 flag to
>> RAW3270_FLAGS_CONSOLE.
>>
>> Fixes: 33403dcfcdfd ("[S390] 3270 console: convert from bootmem to slab")
>> Cc: linux-s390@vger.kernel.org
>> Signed-off-by: Yunseong Kim <yskelg@gmail.com>
>> ---
>>  drivers/s390/char/raw3270.c | 20 +++++++++++++++-----
>>  1 file changed, 15 insertions(+), 5 deletions(-)
> ...
>>  	rc = raw3270_setup_device(cdev, rp, ascebc);
>> -	if (rc)
>> +	if (rc) {
>> +		kfree(ascebc);
>> +		kfree(rp);
>>  		return ERR_PTR(rc);
>> -	set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
>> -
>> +	}
>>  	rc = ccw_device_enable_console(cdev);
>>  	if (rc) {
>>  		ccw_device_destroy_console(cdev);
>> +		kfree(ascebc);
>> +		kfree(rp);
>>  		return ERR_PTR(rc);
>>  	}
>> +	set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
> 
> Why did you move the set_bit() call?

Thank you for the code review Heiko.

While writing patch version 2, I spent a lot of time thinking about this
part. Previously, even if function "ccw_device_enable_console" failed,
the flag was set to RAW3270_FLAGS_CONSOLE and returned.

I think it would be more appropriate to set the bit after everything
succeeded, so I included and submitted this code in v2 patch.

I’d appreciate hearing your thoughts on this!


Warm regards,

Yunseong Kim

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

[Heiko Carstens]

On Tue, Jun 25, 2024 at 04:44:47PM +0900, Yunseong Kim wrote:
> Hi Heiko,
> >> -	set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
> >> -
> >> +	}
> >>  	rc = ccw_device_enable_console(cdev);
> >>  	if (rc) {
> >>  		ccw_device_destroy_console(cdev);
> >> +		kfree(ascebc);
> >> +		kfree(rp);
> >>  		return ERR_PTR(rc);
> >>  	}
> >> +	set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
> > 
> > Why did you move the set_bit() call?
> 
> Thank you for the code review Heiko.
> 
> While writing patch version 2, I spent a lot of time thinking about this
> part. Previously, even if function "ccw_device_enable_console" failed,
> the flag was set to RAW3270_FLAGS_CONSOLE and returned.
> 
> I think it would be more appropriate to set the bit after everything
> succeeded, so I included and submitted this code in v2 patch.
> 
> I’d appreciate hearing your thoughts on this!

"More appropriate" is not a technical reason. Please don't mix
different things into a single patch. If the set_bit() call needs to
be moved then you need to provide a technical reason for it.

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

[Markus Elfring]

>               … Thus add corresponding return value checks.

I suggest to move this sentence into a subsequent text line.


> The allocated each memory areas are immediately overwritten by the called
> function zero-initialisation be omitted by calling the "kmalloc" instead.

It seems that you stumbled on wording difficulties according to my previous
patch review suggestion.
I find the intended change more appropriate for another update step.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n168


> After "ccw_device_enable_console" succeeds, set the bit raw3270 flag to
> RAW3270_FLAGS_CONSOLE.

Why do you find such an adjustment relevant here?


> Fixes: 33403dcfcdfd ("[S390] 3270 console: convert from bootmem to slab")
> Cc: linux-s390@vger.kernel.org

Would you like to specify a “stable tag”?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.10-rc5#n34


> ---

I would appreciate a version description behind the marker line.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n713


…
> +++ b/drivers/s390/char/raw3270.c
> @@ -811,18 +811,28 @@ struct raw3270 __init *raw3270_setup_console(void)
>  	if (IS_ERR(cdev))
>  		return ERR_CAST(cdev);
>
> -	rp = kzalloc(sizeof(*rp), GFP_KERNEL | GFP_DMA);
> -	ascebc = kzalloc(256, GFP_KERNEL);
> +	rp = kmalloc(sizeof(*rp), GFP_KERNEL | GFP_DMA);
> +	if (!rp)
> +		return ERR_PTR(-ENOMEM);
> +	ascebc = kmalloc(256, GFP_KERNEL);
> +	if (!ascebc) {
> +		kfree(rp);
> +		return ERR_PTR(-ENOMEM);
> +	}
>  	rc = raw3270_setup_device(cdev, rp, ascebc);
> -	if (rc)
> +	if (rc) {
> +		kfree(ascebc);
> +		kfree(rp);
>  		return ERR_PTR(rc);
…

Please take further software design options better into account.

A) goto chain
   https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources

B) scope-based resource management
   https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/slab.h#L282


Regards,
Markus