netdev - Re: [PATCH v9 10/27] resource: harden 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: <67902761a4869_20fa294c6@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 21 Jan 2025 15:01:53 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Alejandro Lucero Palau <alucerop@....com>, Dan Williams
	<dan.j.williams@...el.com>, <alejandro.lucero-palau@....com>,
	<linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>, <edward.cree@....com>,
	<davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<edumazet@...gle.com>, <dave.jiang@...el.com>
Subject: Re: [PATCH v9 10/27] resource: harden resource_contains

Alejandro Lucero Palau wrote:
> 
> On 1/18/25 02:03, Dan Williams wrote:
> > alejandro.lucero-palau@ wrote:
> >> From: Alejandro Lucero <alucerop@....com>
> >>
> >> While resource_contains checks for IORESOURCE_UNSET flag for the
> >> resources given, if r1 was initialized with 0 size, the function
> >> returns a false positive. This is so because resource start and
> >> end fields are unsigned with end initialised to size - 1 by current
> >> resource macros.
> >>
> >> Make the function to check for the resource size for both resources
> >> since r2 with size 0 should not be considered as valid for the function
> >> purpose.
> >>
> >> Signed-off-by: Alejandro Lucero <alucerop@....com>
> >> Suggested-by: Alison Schofield <alison.schofield@...el.com>
> >> Reviewed-by: Alison Schofield <alison.schofield@...el.com>
> >> Acked-by: Bjorn Helgaas <bhelgaas@...gle.com>
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> >> ---
> >>   include/linux/ioport.h | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> >> index 5385349f0b8a..7ba31a222536 100644
> >> --- a/include/linux/ioport.h
> >> +++ b/include/linux/ioport.h
> >> @@ -296,6 +296,8 @@ static inline unsigned long resource_ext_type(const struct resource *res)
> >>   /* True iff r1 completely contains r2 */
> >>   static inline bool resource_contains(const struct resource *r1, const struct resource *r2)
> >>   {
> >> +	if (!resource_size(r1) || !resource_size(r2))
> >> +		return false;
> > I just worry that some code paths expect the opposite, that it is ok to
> > pass zero size resources and get a true result.
> 
> 
> That is an interesting point, I would say close to philosophic 
> arguments. I guess you mean the zero size resource being the one that is 
> contained inside the non-zero one, because the other option is making my 
> vision blurry. In fact, even that one makes me feel trapped in a 
> window-less room, in summer, with a bunch of economists, I mean 
> philosophers, and my phone without signal for emergency calls.

The regression risk is not philosophic relative to how long this
function has returned 'true' for the size == 0 case.

> But maybe it is just  my lack of understanding and there exists a good 
> reason for this possibility.

Questions like the following are good to answer when changing long
standing behavior:

Did you look at any other resource_contains() user to get a sense of
that regression risk?

Is the benefit to changing this higher than that risk?

Would it be better to just document the expectation that the caller
should only pass non-zero sized resources to this function?