| Message ID | 20190311070138.24084-1-kjlu@umn.edu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | net: renesas: fix a potential NULL pointer dereference | expand |
Hi Kangjie, On Mon, Mar 11, 2019 at 8:01 AM Kangjie Lu <kjlu@umn.edu> wrote: > In case of_get_phy_mode fails, the fix returns NULL to avoid > the NULL pointer dereference. > > Signed-off-by: Kangjie Lu <kjlu@umn.edu> Thanks for your patch! > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c > @@ -3187,6 +3187,8 @@ static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev) > return NULL; > > pdata->phy_interface = of_get_phy_mode(np); > + if (unlikely(!pdata->phy_interface)) > + return NULL; of_get_phy_mode() does not return a pointer, but a phy mode index. Hence zero is a valid value. So I'm wondering where is the potential NULL pointer dereference? Perhaps we do need to check for negative error codes? Or is this already handled later, by {of_,}phy_connect()? Gr{oetje,eeting}s, Geert
Hello! On 03/11/2019 10:43 AM, Geert Uytterhoeven wrote: >> In case of_get_phy_mode fails, the fix returns NULL to avoid >> the NULL pointer dereference. >> >> Signed-off-by: Kangjie Lu <kjlu@umn.edu> > > Thanks for your patch! > >> --- a/drivers/net/ethernet/renesas/sh_eth.c >> +++ b/drivers/net/ethernet/renesas/sh_eth.c >> @@ -3187,6 +3187,8 @@ static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev) >> return NULL; >> >> pdata->phy_interface = of_get_phy_mode(np); >> + if (unlikely(!pdata->phy_interface)) >> + return NULL; > > of_get_phy_mode() does not return a pointer, but a phy mode index. > Hence zero is a valid value. > > So I'm wondering where is the potential NULL pointer dereference? Hm, not seeing NULL deref as well... > Perhaps we do need to check for negative error codes? > Or is this already handled later, by {of_,}phy_connect()? Not seeing such handling too... The v2 patch seems good though, as the "phy-mode" prop is mandatory anyway. > Gr{oetje,eeting}s, > > Geert MBR, Sergei
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c index 339b2eae2100..b119587ec673 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c @@ -3187,6 +3187,8 @@ static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev) return NULL; pdata->phy_interface = of_get_phy_mode(np); + if (unlikely(!pdata->phy_interface)) + return NULL; mac_addr = of_get_mac_address(np); if (mac_addr)
In case of_get_phy_mode fails, the fix returns NULL to avoid the NULL pointer dereference. Signed-off-by: Kangjie Lu <kjlu@umn.edu> --- drivers/net/ethernet/renesas/sh_eth.c | 2 ++ 1 file changed, 2 insertions(+)