2022-12-08 13:13:42

by Jiasheng Jiang

Subject: [PATCH net v3] ice: Add check for kzalloc

As kzalloc may return NULL pointer, the return value should
be checked and return error if fails in order to avoid the
NULL pointer dereference.
Moreover, use the goto-label to share the clean code.

Fixes: d6b98c8d242a ("ice: add write functionality for GNSS TTY")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
Changelog:

v2 -> v3:

1. Use "while (i--)" to simplify the code.

v1 -> v2:

1. Use goto-label to share the clean code.
---
drivers/net/ethernet/intel/ice/ice_gnss.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index b5a7f246d230..90c620c26516 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -462,6 +462,9 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
GFP_KERNEL);
pf->gnss_serial[i] = NULL;

+ if (!pf->gnss_tty_port[i])
+ goto err_out;
+
tty_port_init(pf->gnss_tty_port[i]);
tty_port_link_device(pf->gnss_tty_port[i], tty_driver, i);
}
@@ -469,21 +472,23 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
err = tty_register_driver(tty_driver);
if (err) {
dev_err(dev, "Failed to register TTY driver err=%d\n", err);
-
- for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
- tty_port_destroy(pf->gnss_tty_port[i]);
- kfree(pf->gnss_tty_port[i]);
- }
- kfree(ttydrv_name);
- tty_driver_kref_put(pf->ice_gnss_tty_driver);
-
- return NULL;
+ goto err_out;
}

for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++)
dev_info(dev, "%s%d registered\n", ttydrv_name, i);

return tty_driver;
+
+err_out:
+ while (i--) {
+ tty_port_destroy(pf->gnss_tty_port[i]);
+ kfree(pf->gnss_tty_port[i]);
+ }
+ kfree(ttydrv_name);
+ tty_driver_kref_put(pf->ice_gnss_tty_driver);
+
+ return NULL;
}

/**
--
2.25.1

2022-12-08 13:37:35

by Jiri Pirko

Subject: Re: [PATCH net v3] ice: Add check for kzalloc

Thu, Dec 08, 2022 at 01:58:47PM CET, [email protected] wrote:
>As kzalloc may return NULL pointer, the return value should
>be checked and return error if fails in order to avoid the
>NULL pointer dereference.

It would be nice if you change the sentence above to imperative mood.
Tell the codebase what to do.


>Moreover, use the goto-label to share the clean code.
>
>Fixes: d6b98c8d242a ("ice: add write functionality for GNSS TTY")
>Signed-off-by: Jiasheng Jiang <[email protected]>
>---
>Changelog:
>
>v2 -> v3:
>
>1. Use "while (i--)" to simplify the code.
>
>v1 -> v2:
>
>1. Use goto-label to share the clean code.
>---
> drivers/net/ethernet/intel/ice/ice_gnss.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
>index b5a7f246d230..90c620c26516 100644
>--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
>+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
>@@ -462,6 +462,9 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
> GFP_KERNEL);
> pf->gnss_serial[i] = NULL;
>
>+ if (!pf->gnss_tty_port[i])
>+ goto err_out;

Move the check right after the alloc.


>+
> tty_port_init(pf->gnss_tty_port[i]);
> tty_port_link_device(pf->gnss_tty_port[i], tty_driver, i);
> }
>@@ -469,21 +472,23 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
> err = tty_register_driver(tty_driver);
> if (err) {
> dev_err(dev, "Failed to register TTY driver err=%d\n", err);
>-
>- for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
>- tty_port_destroy(pf->gnss_tty_port[i]);
>- kfree(pf->gnss_tty_port[i]);
>- }
>- kfree(ttydrv_name);
>- tty_driver_kref_put(pf->ice_gnss_tty_driver);
>-
>- return NULL;
>+ goto err_out;
> }
>
> for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++)
> dev_info(dev, "%s%d registered\n", ttydrv_name, i);
>
> return tty_driver;
>+
>+err_out:
>+ while (i--) {
>+ tty_port_destroy(pf->gnss_tty_port[i]);
>+ kfree(pf->gnss_tty_port[i]);
>+ }
>+ kfree(ttydrv_name);
>+ tty_driver_kref_put(pf->ice_gnss_tty_driver);
>+
>+ return NULL;

This looks fine now.


> }
>
> /**
>--
>2.25.1
>