linux-kernel - Re: [PATCH] resource: handle wrong resource

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: <69360bb3.7b0a0220.46cd2.4675@mx.google.com>
Date: Mon, 8 Dec 2025 00:20:16 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Dan Williams <dan.j.williams@...el.com>,
	Jonathan Cameron <jonathan.cameron@...wei.com>,
	Magnus Damm <damm@...l.co.jp>, linux-kernel@...r.kernel.org,
	"Rob Herring (Arm)" <robh@...nel.org>, stable@...r.kernel.org
Subject: Re: [PATCH] resource: handle wrong resource_size value on zero
 start/end resource

On Mon, Dec 08, 2025 at 01:12:03AM +0200, Andy Shevchenko wrote:
> On Sun, Dec 07, 2025 at 10:53:48PM +0100, Christian Marangi wrote:
> > Commit 900730dc4705 ("wifi: ath: Use
> > of_reserved_mem_region_to_resource() for "memory-region"") uncovered a
> > massive problem with the usage of resource_size() helper.
> > 
> > The reported commit caused a regression with ath11k WiFi firmware
> > loading and the change was just a simple replacement of duplicate code
> > with a new helper of_reserved_mem_region_to_resource().
> > 
> > On reworking this, in the commit also a check for the presence of the
> > node was replaced with resource_size(&res). This was done following the
> > logic that if the node wasn't present then it's expected that also the
> > resource_size is zero, mimicking the same if-else logic.
> > 
> > This was also the reason the regression was mostly hard to catch at
> > first sight as the rework is correctly done given the assumption on the
> > used helpers.
> > 
> > BUT this is actually not the case. On further inspection on
> > resource_size() it was found that it NEVER actually returns 0.
> > 
> > Even if the resource value of start and end are 0, the return value of
> > resource_size() will ALWAYS be 1, resulting in the broken if-else
> > condition ALWAYS going in the first if condition.
> > 
> > This was simply confirmed by reading the resource_size() logic:
> > 
> > 	return res->end - res->start + 1;
> > 
> > Given the confusion, also other case of such usage were searched in the
> > kernel and with great suprise it seems LOTS of place assume
> > resource_size() should return zero in the context of the resource start
> > and end set to 0.
> > 
> > Quoting for example comments in drivers/vfio/pci/vfio_pci_core.c:
> > 
> > 		/*
> > 		 * The PCI core shouldn't set up a resource with a
> > 		 * type but zero size. But there may be bugs that
> > 		 * cause us to do that.
> > 		 */
> > 		if (!resource_size(res))
> > 			goto no_mmap;
> > 
> > It really seems resource_size() was tought with the assumption that
> > resource struct was always correctly initialized before calling it and
> > never set to zero.
> > 
> > But across the year this got lost and now there are lots of driver that
> > assume resource_size() returns 0 if start and end are also 0.
> > 
> > To better handle this and make resource_size() returns correct value in
> > such case, add a simple check and return 0 if both resource start and
> > resource end are zero.
> 
> Good catch!
> 
> Now, let's unveil which drivers rely on "broken" behaviour...
> 
> ...
> 
> >  static inline resource_size_t resource_size(const struct resource *res)
> >  {
> > +	if (!res->start && !res->end)
> > +		return 0;
> 
> I think this breaks or might brake some of the drivers that rely on the proper
> calculation. If you supply the start and end for the same (if it's not 0), you
> will get 1 and it's _correct_ result (surprise surprise). One of the thing that
> may be directly affected (and regress) is the amount of IRQs calculation (which
> on some platforms may start from 0). However, in practice I think it's none
> nowadays in the upstream kernel.
>

One common usage of this is with address size. So if start and end is
the same, then it's ok to have size 1? 

> >  	return res->end - res->start + 1;
> >  }
> 
> That said, unfortunately, I think, you want to fix drivers one-by-one and this
> patch is incorrect as it brings inconsistency to the logic (1 occupied address
> whatever unit it has may still be valid resource).
> 

Yep but probably never aligned? I don't think there is an arch in the
world that is aligned to 1 byte?

> Also a good start is to add test cases and add/update documentation.
> 

I hoped this was simple enough to have the condition. The more
articulate and safe change might be to:
1. rename this to __resource_size
2. rename every entry of resource_size to __resource_size
3. introduce a new resource_size commented and with the check
4. Use the new helper where it's actually needed?

>From my search there are various place where the condition is like:

if (resource_size(&res)) 
   ...

And this condition doesn't make any sense since it's always true (I
highly suspect these case all fall in what I described)

For sure this needs to be discussed and we need to gather more info.

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
	Ansuel