netdev - 回覆: [PATCH net-next] net: ftgmac100: Fix potential NULL dereference in error handling

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] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SEYPR06MB51346AA79267B5EC278F51CA9D9A2@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Tue, 10 Sep 2024 05:55:46 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Uwe Kleine-König
	<u.kleine-koenig@...gutronix.de>, Jacob Keller <jacob.e.keller@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>
Subject:
 回覆: [PATCH net-next] net: ftgmac100: Fix potential NULL dereference in error handling

Hello, Dan,

> > >
> > > We might not have a phy so we need to check for NULL before calling
> > > phy_stop(netdev->phydev) or it could lead to an Oops.
> > >
> > > Fixes: e24a6c874601 ("net: ftgmac100: Get link speed and duplex for
> > > NC-SI")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
> > > ---
> > >  drivers/net/ethernet/faraday/ftgmac100.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > > b/drivers/net/ethernet/faraday/ftgmac100.c
> > > index f3cc14cc757d..0e873e6f60d6 100644
> > > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > > @@ -1565,7 +1565,8 @@ static int ftgmac100_open(struct net_device
> > > *netdev)
> > >  	return 0;
> > >
> > >  err_ncsi:
> > > -	phy_stop(netdev->phydev);
> > > +	if (netdev->phydev)
> > > +		phy_stop(netdev->phydev);
> > When using " use-ncsi" property, the driver will register a fixed-link
> > phy device and bind to netdev at probe stage.
> >
> > if (np && of_get_property(np, "use-ncsi", NULL)) {
> >
> > 		......
> >
> > 		phydev = fixed_phy_register(PHY_POLL, &ncsi_phy_status, NULL);
> > 		err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link,
> > 					 PHY_INTERFACE_MODE_MII);
> 
> This is another bug.  There needs to be error checking in case
> fixed_phy_register() fails, other wise it crashes when we call
> phy_connect_direct().  For example, if the probe() ordering is unlucky
> fixed_phy_register() can return -EPROBE_DEFER so it's not even unusual error
> cases, which can lead to a crash but just normal stuff.
You are right. It is my fault.
I did miss checking the return status of the fixed_phy_register() function.
Thank you for reminding me this bug.

> 
> > 		if (err) {
> > 			dev_err(&pdev->dev, "Connecting PHY failed\n");
> > 			goto err_phy_connect;
> > 		}
> > } else if (np && of_phy_is_fixed_link(np)) {
> >
> > Therefore, it does not need to check if the point is NULL in this error
> handling.
> > Thanks.
> 
> It's really unsafe to assume that we will never add more gotos to the
> ftgmac100_open() function.  If you insist, I could remove the Fixes tag...  Let
> me know.
I will refine this part and send the other patch to fix my bug. Thank you.
> 
> 
> regards,
> dan carpenter