linux-kernel - Re: [PATCH 2/3] PCI: Do not coalesce host bridge resource structs in place

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] [day] [month] [year] [list]

Message-ID: <aO-vtdECWNpYpo6f@smile.fi.intel.com>
Date: Wed, 15 Oct 2025 17:29:09 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: linux-pci@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Kai-Heng Feng <kaihengf@...dia.com>, Rob Herring <robh@...nel.org>,
	linux-kernel@...r.kernel.org,
	Krzysztof Wilczyński <kw@...ux.com>,
	Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH 2/3] PCI: Do not coalesce host bridge resource structs in
 place

On Fri, Oct 10, 2025 at 05:42:30PM +0300, Ilpo Järvinen wrote:
> The resource coalescing for host bridge windows in
> pci_register_host_bridge() can alter the underlying struct resource
> which is inherently dangerous as this code has no knowledge of who else
> holds a pointer the same resource.
> 
> Merge the struct resource inside a newly added
> resource_list_merge_entries() which uses the internal __res member of
> the struct resource_entry to store the merged resource, thus preserving
> the original resource structs.

...

> +static void resource_clear_tree_links(struct resource *res)
> +{
> +	res->parent = NULL;
> +	res->child = NULL;
> +	res->sibling = NULL;
> +}

Not sure if this is the best location to inject a new helper to in the code
(I mean the position in the file). But I leave it to you just to give another
look in case something more suitable can be found.

>  static int __release_resource(struct resource *old, bool release_child)

...

> +/**
> + * resource_mergeable - Test if resources are contiguous and can be merged
> + * @r1: first resource
> + * @r2: second resource
> + *
> + * Tests @r1 is followed by @r2 contiguously and share the metadata.

This needs an additional explanation about name equivalence that's not only by
pointers, but by a content.

> + * Return: %true if resources are mergeable non-destructively.
> + */
> +static bool resource_mergeable(struct resource *r1, struct resource *r2)
> +{
> +	if ((r1->flags != r2->flags) ||
> +	    (r1->desc != r2->desc) ||
> +	    (r1->parent != r2->parent) ||
> +	    (r1->end + 1 != r2->start))
> +		return false;

> +	if (r1->name == r2->name)
> +		return true;
> +
> +	if (r1->name && r2->name && !strcmp(r1->name, r2->name))
> +		return true;
> +
> +	return false;

Hmm... Can we keep the logic more straight as in returning false cases as soon
as possible?

I think of something like this:

	if (r1->name && r2->name)
		return strcmp(r1->name, r2->name) == 0;

	return r1->name == r2->name;

> +}

...

> +	new_res->start = res->start;
> +	new_res->end = next_res->end;
> +	new_res->name = res->name;
> +	new_res->flags = res->flags;
> +	new_res->desc = res->desc;

Hmm... IIRC I saw similar code lines a few times in the kernel in resource.c
and might be elsewhere. Perhaps a time for another helper?


...

> +	/* prepare for step 2), find res & next_res from child/sibling chain. */
> +	p = &parent->child;
> +	while (1) {
> +		tmp = *p;
> +		if (tmp == res)
> +			break;
> +
> +		/*  No res in child/sibling, the resource tree is corrupted! */

Extra space which is not needed.

> +		if (WARN_ON_ONCE(!tmp))
> +			return -EINVAL;
> +
> +		p = &tmp->sibling;
> +	}

...

> static bool system_ram_resources_mergeable(struct resource *r1,
>  					   struct resource *r2)
>  {
>  	/* We assume either r1 or r2 is IORESOURCE_SYSRAM_MERGEABLE. */
> -	return r1->flags == r2->flags && r1->end + 1 == r2->start &&
> -	       r1->name == r2->name && r1->desc == r2->desc &&
> +	return resource_mergeable(r1, r2) &&
>  	       !r1->child && !r2->child;

Now one line?

>  }

> +struct resource_entry *resource_list_merge_entries(struct resource_entry *entry,
> +						   struct resource_entry *next)
> +{
> +	struct resource *res = entry->res, *next_res = next->res, *new_res;
> +	int ret;
> +
> +	if ((entry->offset != next->offset) ||
> +	    !resource_mergeable(res, next_res))

One line? (It's only 82 characters long)

> +		return ERR_PTR(-EINVAL);
> +
> +	/* Use the internal __res to not mutate the input resources. */
> +	struct resource_entry __free(kfree) *new = resource_list_create_entry(NULL, 0);
> +	if (!new)
> +		return ERR_PTR(-ENOMEM);
> +
> +	new->offset = next->offset;
> +	new_res = new->res;
> +
> +	ret = resource_coalesce(res, next_res, new_res);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	resource_list_add_tail(new, &entry->node);
> +	resource_list_destroy_entry(entry);
> +	resource_list_destroy_entry(next);
> +
> +	return no_free_ptr(new);
> +}

-- 
With Best Regards,
Andy Shevchenko