netdev - Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

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: <20260123121529.inik6xrfdianljq6@skbuf>
Date: Fri, 23 Jan 2026 14:15:29 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org,
	Herve Codina <herve.codina@...tlin.com>,
	Mark Brown <broonie@...nel.org>,
	Serge Semin <fancer.lancer@...il.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
	Jiawen Wu <jiawenwu@...stnetic.com>
Subject: Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with
 non-MMIO regmaps

On Fri, Jan 23, 2026 at 09:20:58AM +0200, Andy Shevchenko wrote:
> On Fri, Jan 23, 2026 at 12:18:48AM +0200, Vladimir Oltean wrote:
> > On Thu, Jan 22, 2026 at 04:38:37PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jan 22, 2026 at 03:47:04PM +0200, Vladimir Oltean wrote:
> > > > On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:
> 
> ...
> 
> > > > > > > +	unsigned int base;
> > > > > > 
> > > > > > Hmm... resource_size_t ?
> > > 
> > > > > Well, regmap_read() takes "unsigned int reg".
> > > > > https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/regmap.h#L1297
> > > > > So in practice, a truncation will be done somewhere if the register base
> > > > > exceeds unsigned int storage capacity. But I didn't feel that it's worth
> > > > > handling that.
> > > > 
> > > > Would this address your feedback?
> > > 
> > > Yes and no. See my remarks below.
> 
> ...
> 
> > > > -	if (config->resource)
> > > > +	if (config->resource) {
> > > 
> > > Btw, this might be not enough, one should check size and flags as well
> > > before use. There was a discussion about this recently. Maybe we should
> > > just move to a simple unsigned int in the config for now? Because handling
> > > resources maybe considered as over engineering in this case.
> > 
> > The resource flags are never taken into consideration, but I can for
> > sure replace the resource in struct mdio_regmap_config with just an
> > unsigned int start and an end, but that doesn't get rid of the resource
> > usage. The dev_get_resource(dev->parent, NULL) call is how we learn of
> > where our register window is located in the "one big regmap" provided by
> > the parent (SJA1105). So we still need this check somewhere else if we
> > wanted to not fail silently in case of address bits truncation.
> 
> Hmm... Bu why we can't embed the full struct resource in such a case?

We can also embed the full struct resource, I never said we can't...

> Because resource should have a flag check, otherwise it's a wrong check.
> 
> Discussion I mentioned is this:
> https://lore.kernel.org/lkml/20251207215359.28895-1-ansuelsmth@gmail.com/
> 
> Fixes due to that finding:
> https://lore.kernel.org/lkml/20251208200437.14199-1-ansuelsmth@gmail.com/
> https://lore.kernel.org/lkml/20251208145654.5294-1-ilpo.jarvinen@linux.intel.com/

The linked issues seem unrelated; they are caused by the assumption that
resource_size() can be zero. But I'm not using the resource_size()
helper, and even if I were, I'm not testing it against zero.

As opposed to the PCI BAR case, we don't keep around in an altered form
the resources exceeding 4G. Just need to reject them once and be done
with them.

Also, what else to even check about the resource flags? We get the
resource using "platform_get_resource(pdev, IORESOURCE_REG, 0)", so we
know they're of that type. I don't think IORESOURCE_REG resources have
any other valid bits in flags except for IORESOURCE_TYPE_BITS.

> > > > +		if (config->resource->start > U32_MAX ||
> > > > +		    config->resource->end > U32_MAX) {
> > > 
> > > Ideally it should be resource_overlaps() check. But see above.
> > 
> > resource_overlaps_with_what? The only problem is that the resource can
> > exceed the 32 bit representation that regmap works with.
> 
> Obviously with the 4G address space :-)
> 
> 	struct resource r4g = DEFINE_RESOURCE...(..., 0, SZ_4G...);
> 
> 	if (resource_overlaps(&r4g, config->resource))
> 		aiaiai! // using %pR to print the content

This is a buggy replacement of my intention. I need to sanity check that
my IORESOURCE_REG resource is entirely within the 0-4G region.

The correct way to express this using helpers:

	if (!resource_contains(&r4g, config->resource))
		nazad!

but... you see my point? In trying to make use of "standard" helpers, we
overcomplicate simple things and introduce bugs.

My initially proposed test can be written even simpler:

	if (config->resource->end > U32_MAX) {
		...

because end >= start, so also testing resource->start is redundant.

> > > > +			dev_err(config->parent,
> > > > +				"Resource exceeds regmap API addressing possibilities\n");
> > > > +			return ERR_PTR(-EINVAL);
> > > > +		}
> > > >  		mr->base = config->resource->start;
> > > > +	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>