| Message ID | 20240625024819.26299-2-yskelg@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] s390/netiucv: handle memory allocation failure in conn_action_start() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
> … Thus add a corresponding return value I suggest to move this sentence into a subsequent text line. > … This prevent null pointer dereferenced kernel panic when memory > exhausted situation with the netiucv driver operating as an FSM state > in "conn_action_start". Do you find an improved wording still relevant for a separate paragraph? > Fixes: eebce3856737 ("[S390]: Adapt netiucv driver to new IUCV API") > 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/net/netiucv.c > @@ -855,6 +855,9 @@ 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) > + return; … Would the following statement variant become more appropriate here? + return -ENOMEM; Regards, Markus
On 25.06.24 04:48, yskelg@gmail.com wrote: > From: Yunseong Kim <yskelg@gmail.com> Thank you very much Yunseong, for finding and reporting this potential null-pointer dereference. And thank you Markus Elfring for your valuable comments. > > 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. This prevent null pointer dereferenced kernel panic when memory > exhausted situation with the netiucv driver operating as an FSM state > in "conn_action_start". I would prefer an even shorter commit message. Allocating memory without checking for failure is a common error pattern, I think. > > Fixes: eebce3856737 ("[S390]: Adapt netiucv driver to new IUCV API") > Cc: linux-s390@vger.kernel.org > Signed-off-by: Yunseong Kim <yskelg@gmail.com> > --- > drivers/s390/net/netiucv.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c > index 039e18d46f76..d3ae78c0240f 100644 > --- a/drivers/s390/net/netiucv.c > +++ b/drivers/s390/net/netiucv.c > @@ -855,6 +855,9 @@ 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) > + return; > + On 25.06.24 09:24, Markus Elfring wrote: > > Would the following statement variant become more appropriate here? > > + return -ENOMEM; conn_action_start(), like the other fsm functions, does not have a return value. But simply returning will not prevent the next fsm function from trying to use conn->path. So I think you need to do fsm_newstate(fi, CONN_STATE_CONNERR); before you return. You could use rc = -ENOMEM and a goto to the IUCV_DBF_TEXT_ debug message, if you want to. But I am not too concerned about the details of error handling: If your memory is so scarce that you cannot even allocate a handful of bytes, then you should be seeing warnings all over the place.
diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c index 039e18d46f76..d3ae78c0240f 100644 --- a/drivers/s390/net/netiucv.c +++ b/drivers/s390/net/netiucv.c @@ -855,6 +855,9 @@ 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) + return; + IUCV_DBF_TEXT_(setup, 2, "%s: connecting to %s ...\n", netdev->name, netiucv_printuser(conn));