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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y5HhaLOle2tZuT9S@nanopsycho>
Date:   Thu, 8 Dec 2022 14:06:48 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jiasheng Jiang <jiasheng@...as.ac.cn>
Cc:     leon@...nel.org, jesse.brandeburg@...el.com,
        anthony.l.nguyen@...el.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v3] ice: Add check for kzalloc

Thu, Dec 08, 2022 at 01:58:47PM CET, jiasheng@...as.ac.cn 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 <jiasheng@...as.ac.cn>
>---
>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
>