| Message ID | 41c3ea1df1af4f03b2c66728af6812fb@terma.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ll_temac: platform_get_resource replaced by wrong function | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
+ Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michal Simek, Alex Elder Wei Fang, Uwe Kleine-König, Dan Carpenter, Rob Herring, Wang Hai On Tue, Mar 19, 2024 at 07:45:26PM +0000, Claus Hansen Ries wrote: > From: Claus Hansen ries <chr@terma.com> > > devm_platform_ioremap_resource_byname is called using 0 as name, which eventually > ends up in platform_get_resource_byname, where it causes a null pointer in strcmp. > > if (type == resource_type(r) && !strcmp(r->name, name)) > > The correct function is devm_platform_ioremap_resource. Hi Claus, It is curious that this wasn't noticed earlier - does the driver function in some circumstances without this change? > > Fixes: bd69058 ("net: ll_temac: Use devm_platform_ioremap_resource_byname()") nit: Fixes tags should use 12 or more characters for the hash. Fixes: bd69058f50d5 ("net: ll_temac: Use devm_platform_ioremap_resource_byname()") > Signed-off-by: Claus H. Ries <chr@terma.com> > Cc: stable@vger.kernel.org Unfortunately the patch does not apply - it seems that tabs have been replaced by spaces somewhere along the way. It would be best to repost with that addressed. Using git send-email usually works. Also, as this is a fix, please target it at the net tree. That means it should be based on that tree (that part is fine :) and designated as being for net in the subject. Subject: [PATCH net] ... Lastly, please run get_maintainer.pl on your patch to provide the list of parties to CC. https://docs.kernel.org/process/maintainer-netdev.html > --- > drivers/net/ethernet/xilinx/ll_temac_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c > index 9df39cf8b097..1072e2210aed 100644 > --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > @@ -1443,7 +1443,7 @@ static int temac_probe(struct platform_device *pdev) > } > /* map device registers */ > - lp->regs = devm_platform_ioremap_resource_byname(pdev, 0); > + lp->regs = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(lp->regs)) { > dev_err(&pdev->dev, "could not map TEMAC registers\n"); > return -ENOMEM; > > base-commit: d95fcdf4961d27a3d17e5c7728367197adc89b8d > -- 2.39.3 (Apple Git-146) > > >
Hi,
We ran into the issue when upgrading from kernel 5.4.x to 6.1.x. I don't think it is a much used driver.
Can't say if this would work on different implementations of Xilinx HDL, but looking at the code, I can't see devm_platform_ioremap_resource_byname/platform_get_resource_byname succeed without hitting strcmp, any other path makes the platform_get_resource_byname return NULL and __devm_ioremap_resource fail with "res == NULL" (as far I can see). It would require strcmp being able to survive with the name pointer being equal to 0.
void __iomem *
devm_platform_ioremap_resource_byname(struct platform_device *pdev,
const char *name)
{
struct resource *res;
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
return devm_ioremap_resource(&pdev->dev, res);
}
struct resource *platform_get_resource_byname(struct platform_device *dev,
unsigned int type,
const char *name)
{
u32 i;
for (i = 0; i < dev->num_resources; i++) {
struct resource *r = &dev->resource[i];
if (unlikely(!r->name))
continue;
if (type == resource_type(r) && !strcmp(r->name, name))
return r;
}
return NULL;
}
void __iomem *devm_ioremap_resource(struct device *dev,
const struct resource *res)
{
return __devm_ioremap_resource(dev, res, DEVM_IOREMAP);
}
__devm_ioremap_resource(struct device *dev, const struct resource *res,
enum devm_ioremap_type type)
{
resource_size_t size;
void __iomem *dest_ptr;
char *pretty_name;
BUG_ON(!dev);
if (!res || resource_type(res) != IORESOURCE_MEM) {
dev_err(dev, "invalid resource %pR\n", res);
return IOMEM_ERR_PTR(-EINVAL);
}
....
Claus Hansen Ries
Specialist, Software Engineering
Radar Application Software
Terma A/S
-----Original Message-----
From: Simon Horman <horms@kernel.org>
Sent: 20. marts 2024 12:55
To: Claus Hansen Ries <chr@terma.com>
Cc: netdev@vger.kernel.org; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Michal Simek <michal.simek@amd.com>; Alex Elder <elder@linaro.org>; Wei Fang <wei.fang@nxp.com>; Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; Dan Carpenter <dan.carpenter@linaro.org>; Rob Herring <robh@kernel.org>; Wang Hai <wanghai38@huawei.com>
Subject: Re: [PATCH] net: ll_temac: platform_get_resource replaced by wrong function
CAUTION: This email originated from outside of Terma. Do not click links or open attachments unless you recognize the sender and know the content is safe.
+ Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michal Simek, Alex Elder
Wei Fang, Uwe Kleine-König, Dan Carpenter, Rob Herring, Wang Hai
On Tue, Mar 19, 2024 at 07:45:26PM +0000, Claus Hansen Ries wrote:
> From: Claus Hansen ries <chr@terma.com>
>
> devm_platform_ioremap_resource_byname is called using 0 as name, which
> eventually ends up in platform_get_resource_byname, where it causes a null pointer in strcmp.
>
> if (type == resource_type(r) && !strcmp(r->name,
> name))
>
> The correct function is devm_platform_ioremap_resource.
Hi Claus,
It is curious that this wasn't noticed earlier - does the driver function in some circumstances without this change?
>
> Fixes: bd69058 ("net: ll_temac: Use
> devm_platform_ioremap_resource_byname()")
nit: Fixes tags should use 12 or more characters for the hash.
Fixes: bd69058f50d5 ("net: ll_temac: Use devm_platform_ioremap_resource_byname()")
> Signed-off-by: Claus H. Ries <chr@terma.com>
> Cc: stable@vger.kernel.org
Unfortunately the patch does not apply - it seems that tabs have been replaced by spaces somewhere along the way. It would be best to repost with that addressed. Using git send-email usually works.
Also, as this is a fix, please target it at the net tree.
That means it should be based on that tree (that part is fine :) and designated as being for net in the subject.
Subject: [PATCH net] ...
Lastly, please run get_maintainer.pl on your patch to provide the list of parties to CC.
https://docs.kernel.org/process/maintainer-netdev.html
> ---
> drivers/net/ethernet/xilinx/ll_temac_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c
> b/drivers/net/ethernet/xilinx/ll_temac_main.c
> index 9df39cf8b097..1072e2210aed 100644
> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c
> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
> @@ -1443,7 +1443,7 @@ static int temac_probe(struct platform_device *pdev)
> }
> /* map device registers */
> - lp->regs = devm_platform_ioremap_resource_byname(pdev, 0);
> + lp->regs = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(lp->regs)) {
> dev_err(&pdev->dev, "could not map TEMAC registers\n");
> return -ENOMEM;
>
> base-commit: d95fcdf4961d27a3d17e5c7728367197adc89b8d
> -- 2.39.3 (Apple Git-146)
>
>
>
--
pw-bot: changes-requested
On Tue, 19 Mar 2024 19:45:26 +0000, Claus Hansen Ries wrote: > From: Claus Hansen ries <chr@terma.com> > > devm_platform_ioremap_resource_byname is called using 0 as name, which eventually > ends up in platform_get_resource_byname, where it causes a null pointer in strcmp. > > if (type == resource_type(r) && !strcmp(r->name, name)) > > The correct function is devm_platform_ioremap_resource. > > Fixes: bd69058 ("net: ll_temac: Use devm_platform_ioremap_resource_byname()") > Signed-off-by: Claus H. Ries <chr@terma.com> > Cc: stable@vger.kernel.org This patch LGTM. Before the commit bd69058 ("net: ll_temac: Use devm_platform_ioremap_resource_byname()"), temac_probe() calls platform_get_resource() with the index 0 to get the resource. So we have to use devm_platform_ioremap_resource() with the index 0 here. > --- > drivers/net/ethernet/xilinx/ll_temac_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c > index 9df39cf8b097..1072e2210aed 100644 > --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > @@ -1443,7 +1443,7 @@ static int temac_probe(struct platform_device *pdev) > } > /* map device registers */ > - lp->regs = devm_platform_ioremap_resource_byname(pdev, 0); > + lp->regs = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(lp->regs)) { > dev_err(&pdev->dev, "could not map TEMAC registers\n"); > return -ENOMEM; However, it seems that the patch is indented by spaces instead of tabs (maybe your mail client replaced this?). I recommend running checkpatch.pl before submitting patches. Also, we should put appropriate prefix, i.e. "net" or "net-next", in the subject. As for this patch, I think "[PATCH net]" is appropriate. Thanks, Shigeru > base-commit: d95fcdf4961d27a3d17e5c7728367197adc89b8d > -- 2.39.3 (Apple Git-146) > > >
On Wed, Mar 20, 2024 at 11:54:33AM +0000, Simon Horman wrote: > > --- > > drivers/net/ethernet/xilinx/ll_temac_main.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c > > index 9df39cf8b097..1072e2210aed 100644 > > --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > > +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > > @@ -1443,7 +1443,7 @@ static int temac_probe(struct platform_device *pdev) > > } > > /* map device registers */ > > - lp->regs = devm_platform_ioremap_resource_byname(pdev, 0); > > + lp->regs = devm_platform_ioremap_resource(pdev, 0); This should have triggered a Sparse warning "warning: Using plain integer as NULL pointer" but the problem is that this file does not have correct endian annotations and after a certain number of warnings Sparse gives up. It's a bit tricky to check for this in Smatch because it's not dereferenced unconditionally. Perhaps instead of asking "Does this function always dereferences the parameter?" Smatch would ask, "Can this function succeed with a NULL parameter?" I don't know... And even that might not help here because the success path is complicated. I can hard code this as a dereferenced parameter by adding it to smatch_dereferences.c. { "devm_platform_ioremap_resource_byname", 1, "$" }, But adding functions one by one doesn't scale. The other thing is that this kind of bug is normally caught in testing so it's not really suited for static analysis. Normally the warnings mean something weird is happening like it's COMPILE_TEST only code. The common false positive is that the dereference is several steps away and the function call table hasn't rebuilt enough to know that passing a NULL used to be illegal but it's allowed now. Looking at the warnings there is only one false positive: net/netfilter/x_tables.c:1630 xt_mttg_seq_start() error: NULL dereference inside function 'xt_mttg_seq_next(seq, (0), (0), is_target)()'. '0' '(0)' 49 9 I'll investigate that. The rest seem like real bugs. drivers/scsi/pcmcia/qlogic_stub.c:274 qlogic_resume() error: NULL dereference inside function 'qlogicfas408_host_reset((0))()'. '0' '(0)' 33 9 drivers/net/ethernet/cavium/liquidio/lio_main.c:810 liquidio_watchdog() error: NULL dereference inside function 'module_refcount((0))()'. '0' '(0)' 44 9 drivers/net/ethernet/nxp/lpc_eth.c:1401 lpc_eth_drv_probe() error: NULL dereference inside function 'lpc32xx_return_iram((0), (0))()'. '0' '(0)' 62 9 drivers/net/ethernet/nxp/lpc_eth.c:1401 lpc_eth_drv_probe() error: NULL dereference inside function 'lpc32xx_return_iram((0), (0))()'. '0' '(0)' 56 9 drivers/net/ethernet/nxp/lpc_eth.c:1428 lpc_eth_drv_remove() error: NULL dereference inside function 'lpc32xx_return_iram((0), (0))()'. '0' '(0)' 62 9 drivers/net/ethernet/nxp/lpc_eth.c:1428 lpc_eth_drv_remove() error: NULL dereference inside function 'lpc32xx_return_iram((0), (0))()'. '0' '(0)' 56 9 net/rxrpc/io_thread.c:454 rxrpc_io_thread() error: NULL dereference inside function 'rxrpc_input_conn_event(conn, (0))()'. '0' '(0)' 54 9 regards, dan carpenter
On Wed, Mar 20, 2024 at 10:13 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Wed, Mar 20, 2024 at 11:54:33AM +0000, Simon Horman wrote: > > > --- > > > drivers/net/ethernet/xilinx/ll_temac_main.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c > > > index 9df39cf8b097..1072e2210aed 100644 > > > --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > > > +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > > > @@ -1443,7 +1443,7 @@ static int temac_probe(struct platform_device *pdev) > > > } > > > /* map device registers */ > > > - lp->regs = devm_platform_ioremap_resource_byname(pdev, 0); > > > + lp->regs = devm_platform_ioremap_resource(pdev, 0); > > This should have triggered a Sparse warning "warning: Using plain > integer as NULL pointer" but the problem is that this file does not have > correct endian annotations and after a certain number of warnings Sparse > gives up. It did[1] along with linker errors on s390. Just no one was CC'ed other than the author. I guess 0-day gave up after a while. Rob [1] https://lore.kernel.org/all/202008070052.yH1Em3c1%25lkp@intel.com/
diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c index 9df39cf8b097..1072e2210aed 100644 --- a/drivers/net/ethernet/xilinx/ll_temac_main.c +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c @@ -1443,7 +1443,7 @@ static int temac_probe(struct platform_device *pdev) } /* map device registers */ - lp->regs = devm_platform_ioremap_resource_byname(pdev, 0); + lp->regs = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(lp->regs)) { dev_err(&pdev->dev, "could not map TEMAC registers\n"); return -ENOMEM;