| Message ID | bc8422a8bf3ff99809413eb62dd12aacc85a9950.1683356951.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] x86/PCI: Fix a sanity check in pirq_convert_irt_table() | expand |
On Sat, 6 May 2023, Christophe JAILLET wrote: > We compare the size in bytes of a struct (and its ending flexible array) > with the number of elements in a flexible array. Incorrect, see the inline documentation for the struct. > This is wrong and "ir->size < ir->used" is likely to be always false. Hopefully, but we've seen all kinds of rubbish in PC BIOS data, and this data structure seems available for OEMs to program with a tool called BCP. Better safe than sorry. Therefore, NAK. Maciej
Le 19/05/2023 à 13:21, Maciej W. Rozycki a écrit : > On Sat, 6 May 2023, Christophe JAILLET wrote: > >> We compare the size in bytes of a struct (and its ending flexible array) >> with the number of elements in a flexible array. > > Incorrect, see the inline documentation for the struct. Ouch. As you explained in your reply for the 2nd patch: irT_routing_table != irQ_routing_table Sorry for the noise. CJ > >> This is wrong and "ir->size < ir->used" is likely to be always false. > > Hopefully, but we've seen all kinds of rubbish in PC BIOS data, and this > data structure seems available for OEMs to program with a tool called BCP. > Better safe than sorry. Therefore, NAK. > > Maciej >
diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c index a498b847d740..e29b487cc069 100644 --- a/arch/x86/pci/irq.c +++ b/arch/x86/pci/irq.c @@ -128,12 +128,16 @@ static inline struct irq_routing_table *pirq_convert_irt_table(u8 *addr, { struct irt_routing_table *ir; struct irq_routing_table *rt; + int entries; u16 size; u8 sum; int i; ir = (struct irt_routing_table *)addr; - if (ir->signature != IRT_SIGNATURE || !ir->used || ir->size < ir->used) + entries = (ir->size - sizeof(struct irq_routing_table)) / + sizeof(struct irq_info); + + if (ir->signature != IRT_SIGNATURE || !ir->used || entries < ir->used) return NULL; size = sizeof(*ir) + ir->used * sizeof(ir->slots[0]);
We compare the size in bytes of a struct (and its ending flexible array) with the number of elements in a flexible array. This is wrong and "ir->size < ir->used" is likely to be always false. Compute the number of possible entries instead, as already done in other places, and compare it to the number of used entries. Fixes: b584db0c84db ("x86/PCI: Add $IRT PIRQ routing table support") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- /!\ This patch is speculative. Review with care /!\ I'm not sure that this sanity check is needed at all. If 'used' was the size of the flexible array, I think it would simplify the code in other places. It will also help scripts when __counted_by macro will be added. See [1]. [1]: https://lore.kernel.org/all/6453f739.170a0220.62695.7785@mx.google.com/ --- arch/x86/pci/irq.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)