[PATCH] s390/netiucv: handle memory allocation failure in conn_action_start()

[PATCH] s390/netiucv: handle memory allocation failure in conn_action_start()

[yskelg]

From: Yunseong Kim <yskelg@gmail.com>

This patch handle potential null pointer dereference in
iucv_path_connect(), When iucv_path_alloc() fails to allocate memory
for 'rc'.

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

diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c
index 039e18d46f76..c2df0c312d81 100644
--- a/drivers/s390/net/netiucv.c
+++ b/drivers/s390/net/netiucv.c
@@ -855,6 +855,10 @@ static void conn_action_start(fsm_instance *fi, int event, void *arg)
 
 	fsm_newstate(fi, CONN_STATE_SETUPWAIT);
 	conn->path = iucv_path_alloc(NETIUCV_QUEUELEN_DEFAULT, 0, GFP_KERNEL);
+	if (!conn->path) {
+		IUCV_DBF_TEXT_(setup, 2, "iucv_path_alloc: memory allocation failed.\n");
+		return;
+	}
 	IUCV_DBF_TEXT_(setup, 2, "%s: connecting to %s ...\n",
 		netdev->name, netiucv_printuser(conn));
 
-- 
2.45.2

Re: [PATCH] s390/netiucv: handle memory allocation failure in conn_action_start()

[Markus Elfring]

> This patch handle potential null pointer dereference in
> iucv_path_connect(), When iucv_path_alloc() fails to allocate memory
> for 'rc'.

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

   A null pointer is stored in the data structure member “path” after a call
   of the function “iucv_path_alloc” failed. This pointer was passed to
   a subsequent call of the function “iucv_path_connect” where an undesirable
   dereference will be performed then.
   Thus add a corresponding return value check.


2. May the proposed error message be omitted
   (because a memory allocation failure might have been reported
   by an other function call already)?


3. Is there a need to adjust the return type of the function “conn_action_start”?


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


5. 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/netiucv: handle memory allocation failure in conn_action_start()

[Yunseong Kim]

Hi Markus,

On 6/23/24 11:27 오후, Markus Elfring wrote:
>> This patch handle potential null pointer dereference in
>> iucv_path_connect(), When iucv_path_alloc() fails to allocate memory
>> for 'rc'.
> 
> 1. Can a wording approach (like the following) be a better change description?
> 
>    A null pointer is stored in the data structure member “path” after a call
>    of the function “iucv_path_alloc” failed. This pointer was passed to
>    a subsequent call of the function “iucv_path_connect” where an undesirable
>    dereference will be performed then.
>    Thus add a corresponding return value check.

Thank you very much for your detailed code review. I will thoughtfully
incorporate your advices into the next patch.

> 2. May the proposed error message be omitted
>    (because a memory allocation failure might have been reported
>    by an other function call already)?

I agree.

> 3. Is there a need to adjust the return type of the function “conn_action_start”?

I had the same thoughts while writing the code. Thank you!

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

Yes, I will refer to the mailing list and include the fix tag.

> 5. 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

I am considering the environment in which the micro Virtual Machine
operates and testing the s390 architecture with QEMU on my Mac M2 PC.
I have been reviewing the code under the assumption of using a lot of
memory and having many micro Virtual Machines loaded simultaneously.

> Regards,
> Markus

I really appreciate code review Markus!


Best regards,

Yunseong Kim

Re: [PATCH] s390/netiucv: handle memory allocation failure in conn_action_start()

[Alexandra Winter]

On 24.06.24 20:00, Yunseong Kim wrote:
>> 5. 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
> I am considering the environment in which the micro Virtual Machine
> operates and testing the s390 architecture with QEMU on my Mac M2 PC.
> I have been reviewing the code under the assumption of using a lot of
> memory and having many micro Virtual Machines loaded simultaneously.
> 
>> Regards,
>> Markus
> I really appreciate code review Markus!
> 
> 
> Best regards,
> 
> Yunseong Kim

(answering the V1 thread first. Content related comments will follow in V2 thread)

s390/netiucv is more or less in maintenance mode and we are not aware of any users.
The enterprise distros do not provide this module. Other iucv modules are more popular.
But afaiu we cannot remove the source code, unless we can prove that nobody is using it.
(Community advice is welcome).

Yunseong, I'd be interested in why you are running this module with QEMU on your
Mac PC?

Alexandra

BTW, your full name is shown in this reply, but not when you send
the patches. See:
https://lore.kernel.org/lkml/?q=s390%2Fnetiucv%3A+handle+memory+allocation+failure+in+conn_action_start%28%29
You may want to check your settings.

Re: [PATCH] s390/netiucv: handle memory allocation failure in conn_action_start()

[Jakub Kicinski]

On Tue, 25 Jun 2024 11:08:49 +0200 Alexandra Winter wrote:
> s390/netiucv is more or less in maintenance mode and we are not aware of any users.
> The enterprise distros do not provide this module. Other iucv modules are more popular.
> But afaiu we cannot remove the source code, unless we can prove that nobody is using it.
> (Community advice is welcome).

If you have strong reason to believe this driver is unused, and can't
find any proof otherwise - let's remove it. We can always "revert it
back in", if needed. We have done it in the past.