| Message ID | 3f196da5-2c1a-4f94-9ced-35d302c1a2b9@stanley.mountain (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ftgmac100: Fix potential NULL dereference in error handling | expand |
On Thu, Sep 5, 2024 at 11:44 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > 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@linaro.org> > --- Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Hello, > > 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@linaro.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); 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. > napi_disable(&priv->napi); > netif_stop_queue(netdev); > err_alloc: > -- > 2.45.2
On Fri, Sep 06, 2024 at 06:06:14AM +0000, Jacky Chou wrote: > Hello, > > > > > 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@linaro.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); > 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. Are you actually saying: if (netdev->phydev) { /* If we have a PHY, start polling */ phy_start(netdev->phydev); } is wrong, it is guaranteed there is always a phydev? Andrew
Hello, > On Fri, Sep 06, 2024 at 06:06:14AM +0000, Jacky Chou wrote: > > Hello, > > > > > > > > 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@linaro.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); > > 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. > > Are you actually saying: > > if (netdev->phydev) { > /* If we have a PHY, start polling */ > phy_start(netdev->phydev); > } > > is wrong, it is guaranteed there is always a phydev? > This patch is focus on error handling when using NC-SI at open stage. if (netdev->phydev) { /* If we have a PHY, start polling */ phy_start(netdev->phydev); } This code is used to check the other cases. Perhaps, phy-handle or fixed-link property are not added in DTS. Thanks. Jacky
> > Are you actually saying: > > > > if (netdev->phydev) { > > /* If we have a PHY, start polling */ > > phy_start(netdev->phydev); > > } > > > > is wrong, it is guaranteed there is always a phydev? > > > This patch is focus on error handling when using NC-SI at open stage. > > if (netdev->phydev) { > /* If we have a PHY, start polling */ > phy_start(netdev->phydev); > } > > This code is used to check the other cases. > Perhaps, phy-handle or fixed-link property are not added in DTS. I'm guessing, but i think the static analysers see this condition, and deducing that phydev might be a NULL. Hence when phy_stop() is called, it needs the check. You say the static analyser is wrong, probably because it cannot check the bigger context. It can be NULL for phy_start() but not for phy_stop(). Maybe you can give it some more hints? Dan, is this Smatch? Is it possible to dump the paths through the code where it thinks it might be NULL? Andrew
On Fri, Sep 06, 2024 at 06:06:14AM +0000, Jacky Chou wrote: > Hello, > > > > > 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@linaro.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. > 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. regards, dan carpenter
On Mon, Sep 09, 2024 at 02:03:32PM +0200, Andrew Lunn wrote: > > > Are you actually saying: > > > > > > if (netdev->phydev) { > > > /* If we have a PHY, start polling */ > > > phy_start(netdev->phydev); > > > } > > > > > > is wrong, it is guaranteed there is always a phydev? > > > > > This patch is focus on error handling when using NC-SI at open stage. > > > > if (netdev->phydev) { > > /* If we have a PHY, start polling */ > > phy_start(netdev->phydev); > > } > > > > This code is used to check the other cases. > > Perhaps, phy-handle or fixed-link property are not added in DTS. > > I'm guessing, but i think the static analysers see this condition, and > deducing that phydev might be a NULL. Hence when phy_stop() is called, > it needs the check. > > You say the static analyser is wrong, probably because it cannot check > the bigger context. It can be NULL for phy_start() but not for > phy_stop(). Maybe you can give it some more hints? > > Dan, is this Smatch? Is it possible to dump the paths through the code > where it thinks it might be NULL? Adding a check here is the correct thing. The current code works because we only have the one goto after the call to phy_start(netdev->phydev), but as soon as we add a second goto then it will crash. Silencing this warning means tying the information from probe() into it. It's a fun problem but not something I'm going to do this year. regards, dan carpenter
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@linaro.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
Hello, Dan > On Mon, Sep 09, 2024 at 02:03:32PM +0200, Andrew Lunn wrote: > > > > Are you actually saying: > > > > > > > > if (netdev->phydev) { > > > > /* If we have a PHY, start polling */ > > > > phy_start(netdev->phydev); > > > > } > > > > > > > > is wrong, it is guaranteed there is always a phydev? > > > > > > > This patch is focus on error handling when using NC-SI at open stage. > > > > > > if (netdev->phydev) { > > > /* If we have a PHY, start polling */ > > > phy_start(netdev->phydev); > > > } > > > > > > This code is used to check the other cases. > > > Perhaps, phy-handle or fixed-link property are not added in DTS. > > > > I'm guessing, but i think the static analysers see this condition, and > > deducing that phydev might be a NULL. Hence when phy_stop() is called, > > it needs the check. > > > > You say the static analyser is wrong, probably because it cannot check > > the bigger context. It can be NULL for phy_start() but not for > > phy_stop(). Maybe you can give it some more hints? > > > > Dan, is this Smatch? Is it possible to dump the paths through the code > > where it thinks it might be NULL? > > Adding a check here is the correct thing. The current code works because we > only have the one goto after the call to phy_start(netdev->phydev), but as soon > as we add a second goto then it will crash. Could you share more detail about the crash is happening when you add a second goto? I'm wondering if there are other things I missed. Thank you. > > Silencing this warning means tying the information from probe() into it. It's a > fun problem but not something I'm going to do this year. > Jacky
On Tue, Sep 10, 2024 at 06:19:54AM +0000, Jacky Chou wrote: > Hello, Dan > > > On Mon, Sep 09, 2024 at 02:03:32PM +0200, Andrew Lunn wrote: > > > > > Are you actually saying: > > > > > > > > > > if (netdev->phydev) { > > > > > /* If we have a PHY, start polling */ > > > > > phy_start(netdev->phydev); > > > > > } > > > > > > > > > > is wrong, it is guaranteed there is always a phydev? > > > > > > > > > This patch is focus on error handling when using NC-SI at open stage. > > > > > > > > if (netdev->phydev) { > > > > /* If we have a PHY, start polling */ > > > > phy_start(netdev->phydev); > > > > } > > > > > > > > This code is used to check the other cases. > > > > Perhaps, phy-handle or fixed-link property are not added in DTS. > > > > > > I'm guessing, but i think the static analysers see this condition, and > > > deducing that phydev might be a NULL. Hence when phy_stop() is called, > > > it needs the check. > > > > > > You say the static analyser is wrong, probably because it cannot check > > > the bigger context. It can be NULL for phy_start() but not for > > > phy_stop(). Maybe you can give it some more hints? > > > > > > Dan, is this Smatch? Is it possible to dump the paths through the code > > > where it thinks it might be NULL? > > > > Adding a check here is the correct thing. The current code works because we > > only have the one goto after the call to phy_start(netdev->phydev), but as soon > > as we add a second goto then it will crash. > > Could you share more detail about the crash is happening when you add a second goto? > I'm wondering if there are other things I missed. I'm saying if we add a feature in the future. Something like this. regards, dan carpenter diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index f3cc14cc757d..417c7f4dd471 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -1562,10 +1562,22 @@ static int ftgmac100_open(struct net_device *netdev) goto err_ncsi; } + ret = some_new_feature(); + if (ret) + goto err_free_ncsi; + return 0; +err_free_ncsi: + if (priv->use_ncsi) + ncsi_stop_dev(priv->ndev); err_ncsi: phy_stop(netdev->phydev); ^^^^^^^^^^^^^^ Crash. napi_disable(&priv->napi); netif_stop_queue(netdev); err_alloc:
Hi Dan, > > > > Could you share more detail about the crash is happening when you add a > second goto? > > I'm wondering if there are other things I missed. > > I'm saying if we add a feature in the future. Something like this. > > regards, > dan carpenter > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > b/drivers/net/ethernet/faraday/ftgmac100.c > index f3cc14cc757d..417c7f4dd471 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -1562,10 +1562,22 @@ static int ftgmac100_open(struct net_device > *netdev) > goto err_ncsi; > } > > + ret = some_new_feature(); > + if (ret) > + goto err_free_ncsi; > + > return 0; > > +err_free_ncsi: > + if (priv->use_ncsi) > + ncsi_stop_dev(priv->ndev); > err_ncsi: > phy_stop(netdev->phydev); > ^^^^^^^^^^^^^^ > Crash. > > napi_disable(&priv->napi); > netif_stop_queue(netdev); > err_alloc: Thank you for the information. I agree with this change. Thanks, Jacky
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); napi_disable(&priv->napi); netif_stop_queue(netdev); err_alloc:
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@linaro.org> --- drivers/net/ethernet/faraday/ftgmac100.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)