net: stmmac: Check NULL ptr on lvts_data in qcom_ethqos_probe()

[PATCH] net: stmmac: Check NULL ptr on lvts_data in qcom_ethqos_probe()

Posted by Ma Ke 1 year, 5 months ago

of_device_get_match_data() can return NULL if of_match_device failed, and
the pointer 'data' was dereferenced without checking against NULL. Add
checking of pointer 'data' in qcom_ethqos_probe().

Cc: stable@vger.kernel.org
Fixes: a7c30e62d4b8 ("net: stmmac: Add driver for Qualcomm ethqos")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 901a3c1959fa..f18393fe58a4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -838,6 +838,9 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 	ethqos->mac_base = stmmac_res.addr;
 
 	data = of_device_get_match_data(dev);
+	if (!data)
+		return -ENODEV;
+
 	ethqos->por = data->por;
 	ethqos->num_por = data->num_por;
 	ethqos->rgmii_config_loopback_en = data->rgmii_config_loopback_en;
-- 
2.25.1

Re: [PATCH] net: stmmac: Check NULL ptr on lvts_data in qcom_ethqos_probe()

Posted by Simon Horman 1 year, 5 months ago

On Wed, Aug 21, 2024 at 09:19:49PM +0800, Ma Ke wrote:
> of_device_get_match_data() can return NULL if of_match_device failed, and
> the pointer 'data' was dereferenced without checking against NULL. Add
> checking of pointer 'data' in qcom_ethqos_probe().
> 
> Cc: stable@vger.kernel.org
> Fixes: a7c30e62d4b8 ("net: stmmac: Add driver for Qualcomm ethqos")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>

Hi Ma Ke,

There is probably no need to repost just because of this.
But as a fix for Networking code it should be targeted at the net tree.

	Subject: [PATCH net] ...

> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 901a3c1959fa..f18393fe58a4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -838,6 +838,9 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	ethqos->mac_base = stmmac_res.addr;
>  
>  	data = of_device_get_match_data(dev);
> +	if (!data)
> +		return -ENODEV;
> +

In this function dev_err_probe() is used, I assume in cases
where a function that returns an error does not emit any logs.

For consistency, perhaps that is appropriate here too?

>  	ethqos->por = data->por;
>  	ethqos->num_por = data->num_por;
>  	ethqos->rgmii_config_loopback_en = data->rgmii_config_loopback_en;
> -- 
> 2.25.1

Re: [PATCH] net: stmmac: Check NULL ptr on lvts_data in qcom_ethqos_probe()

Posted by Bjorn Andersson 1 year, 5 months ago

On Wed, Aug 21, 2024 at 06:37:30PM +0100, Simon Horman wrote:
> On Wed, Aug 21, 2024 at 09:19:49PM +0800, Ma Ke wrote:
> > of_device_get_match_data() can return NULL if of_match_device failed, and
> > the pointer 'data' was dereferenced without checking against NULL. Add
> > checking of pointer 'data' in qcom_ethqos_probe().
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: a7c30e62d4b8 ("net: stmmac: Add driver for Qualcomm ethqos")
> > Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> 
> Hi Ma Ke,
> 
> There is probably no need to repost just because of this.
> But as a fix for Networking code it should be targeted at the net tree.
> 
> 	Subject: [PATCH net] ...
> 
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > index 901a3c1959fa..f18393fe58a4 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > @@ -838,6 +838,9 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
> >  	ethqos->mac_base = stmmac_res.addr;
> >  
> >  	data = of_device_get_match_data(dev);
> > +	if (!data)
> > +		return -ENODEV;
> > +
> 
> In this function dev_err_probe() is used, I assume in cases
> where a function that returns an error does not emit any logs.
> 
> For consistency, perhaps that is appropriate here too?
> 

Unless I'm missing something here this function can only ever be invoked
by a match against one of the entries in qcom_ethqos_match[], which all
of them have a non-NULL data pointer.

As such, if we somehow arrive here with data of NULL, the NULL pointer
dereference on the next line will provide a welcome large splat and a
callstack indicating that we have a problem.


If there's some use case I'm missing, I would prefer if this was
documented in the commit message.

Regards,
Bjorn

> >  	ethqos->por = data->por;
> >  	ethqos->num_por = data->num_por;
> >  	ethqos->rgmii_config_loopback_en = data->rgmii_config_loopback_en;
> > -- 
> > 2.25.1