linux-kernel - Re: [PATCH] drivers: pci: Fix flexible array usage

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: <Z6u-pwlktLnPZNF-@kbusch-mbp>
Date: Tue, 11 Feb 2025 14:18:31 -0700
From: Keith Busch <kbusch@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Purva Yeshi <purvayeshi550@...il.com>, bhelgaas@...gle.com,
	skhan@...uxfoundation.org, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Alex Williamson <alex.williamson@...hat.com>
Subject: Re: [PATCH] drivers: pci: Fix flexible array usage

On Tue, Feb 11, 2025 at 03:02:35PM -0600, Bjorn Helgaas wrote:
> This is kind of a complicated data structure.  IIUC, a struct
> pci_saved_state is allocated only in pci_store_saved_state(), where
> the size is determined by the sum of the sizes of all the entries in
> the dev->saved_cap_space list.
> 
> The pci_saved_state is filled by copying from entries in the
> dev->saved_cap_space list.  The entries need not be all the same size
> because we copy each entry manually based on its size.
> 
> So cap[] is really just the base of this buffer of variable-sized
> entries.  Maybe "struct pci_cap_saved_data cap[]" is not the best
> representation of this, but *cap (a pointer) doesn't seem better.

The original code is actually correct despite using a flexible array of
a struct that contains a flexible array. That arrangement just means you
can't index into it, but the code is only doing pointer arithmetic, so
should be fine.

With this struct:

struct pci_saved_state {
 	u32 config_space[16];
	struct pci_cap_saved_data cap[];
};

Accessing "cap" field returns the address right after the config_space[]
member. When it's changed to a pointer type, though, it needs to be
initialized to *something* but the code doesn't do that. The code just
expects the cap to follow right after the config.

Anyway, to silence the warning we can just make it an anonymous member
and add 1 to the state to get to the same place in memory as before.

---
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a37..e562037644fd0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1929,7 +1929,6 @@ EXPORT_SYMBOL(pci_restore_state);
 
 struct pci_saved_state {
 	u32 config_space[16];
-	struct pci_cap_saved_data cap[];
 };
 
 /**
@@ -1961,7 +1960,7 @@ struct pci_saved_state *pci_store_saved_state(struct pci_dev *dev)
 	memcpy(state->config_space, dev->saved_config_space,
 	       sizeof(state->config_space));
 
-	cap = state->cap;
+	cap = (void *)(state + 1);
 	hlist_for_each_entry(tmp, &dev->saved_cap_space, next) {
 		size_t len = sizeof(struct pci_cap_saved_data) + tmp->cap.size;
 		memcpy(cap, &tmp->cap, len);
@@ -1991,7 +1990,7 @@ int pci_load_saved_state(struct pci_dev *dev,
 	memcpy(dev->saved_config_space, state->config_space,
 	       sizeof(state->config_space));
 
-	cap = state->cap;
+	cap = (void *)(state + 1);
 	while (cap->size) {
 		struct pci_cap_saved_state *tmp;
 
--