| Message ID | ZRnf6wVOu0IJQ2Ok@work |
|---|---|
| State | New |
| Headers | show |
| Series | [next] PCI/P2PDMA: Fix undefined behavior bug in struct pci_p2pdma_pagemap | expand |
On 2023-10-01 15:08, Gustavo A. R. Silva wrote: > `struct dev_pagemap` is a flexible structure, which means that it > contains a flexible-array member at the bottom. This could potentially > lead to an overwrite of the objects following `pgmap` in `struct > pci_p2pdma_pagemap`, when `nr_range > 1`. > > Fix this by placing the declaration of object `pgmap` at the end of > `struct pci_p2pdma_pagemap`. > > -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting > ready to enable it globally. > > Fixes: 0afea3814358 ("PCI/P2PDMA: Add provider's pci_dev to pci_p2pdma_pagemap struct") > Fixes: a6e6fe6549f6 ("PCI/P2PDMA: Introduce private pagemap structure") > Cc: stable@vger.kernel.org > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Makes sense to me, thanks. Although, I'm not sure the fixes tags are appropriate. The flexible-array member was introduced in 5.10 (b7b3c01b19) and both the "fixed" commits predate that change by a number of releases. Also, it's probably worth noting in the commit message that the p2pdma code hardcodes nr_ranges to 1 (in pci_p2pdma_add_resource); so there is no way to actually hit any bug with the current code. I totally agree that the patch should be applied to prevent possible bugs being introduced in the future: Reviewed-by: Logan Gunthorpe <logang@deltatee.com> Logan
On 10/2/23 18:07, Logan Gunthorpe wrote: > > > On 2023-10-01 15:08, Gustavo A. R. Silva wrote: >> `struct dev_pagemap` is a flexible structure, which means that it >> contains a flexible-array member at the bottom. This could potentially >> lead to an overwrite of the objects following `pgmap` in `struct >> pci_p2pdma_pagemap`, when `nr_range > 1`. >> >> Fix this by placing the declaration of object `pgmap` at the end of >> `struct pci_p2pdma_pagemap`. >> >> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting >> ready to enable it globally. >> >> Fixes: 0afea3814358 ("PCI/P2PDMA: Add provider's pci_dev to pci_p2pdma_pagemap struct") >> Fixes: a6e6fe6549f6 ("PCI/P2PDMA: Introduce private pagemap structure") >> Cc: stable@vger.kernel.org >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > > > Makes sense to me, thanks. > > Although, I'm not sure the fixes tags are appropriate. The > flexible-array member was introduced in 5.10 (b7b3c01b19) and both the > "fixed" commits predate that change by a number of releases. You're right. I'll remove those tags. > > Also, it's probably worth noting in the commit message that the p2pdma > code hardcodes nr_ranges to 1 (in pci_p2pdma_add_resource); so there is > no way to actually hit any bug with the current code. Yep. I mention that in this part "This could potentially lead to an overwrite of the objects following `pgmap` in `struct pci_p2pdma_pagemap`, when `nr_range > 1`." > > I totally agree that the patch should be applied to prevent possible > bugs being introduced in the future: > > Reviewed-by: Logan Gunthorpe <logang@deltatee.com> Thanks for the RB and the feedback. :) -- Gustavo
On 2023-10-02 12:40, Gustavo A. R. Silva wrote: > > > On 10/2/23 18:07, Logan Gunthorpe wrote: >> >> >> On 2023-10-01 15:08, Gustavo A. R. Silva wrote: >>> `struct dev_pagemap` is a flexible structure, which means that it >>> contains a flexible-array member at the bottom. This could potentially >>> lead to an overwrite of the objects following `pgmap` in `struct >>> pci_p2pdma_pagemap`, when `nr_range > 1`. >>> >>> Fix this by placing the declaration of object `pgmap` at the end of >>> `struct pci_p2pdma_pagemap`. >>> >>> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting >>> ready to enable it globally. >>> >>> Fixes: 0afea3814358 ("PCI/P2PDMA: Add provider's pci_dev to pci_p2pdma_pagemap struct") >>> Fixes: a6e6fe6549f6 ("PCI/P2PDMA: Introduce private pagemap structure") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> >> >> Makes sense to me, thanks. >> >> Although, I'm not sure the fixes tags are appropriate. The >> flexible-array member was introduced in 5.10 (b7b3c01b19) and both the >> "fixed" commits predate that change by a number of releases. > > You're right. I'll remove those tags. > >> >> Also, it's probably worth noting in the commit message that the p2pdma >> code hardcodes nr_ranges to 1 (in pci_p2pdma_add_resource); so there is >> no way to actually hit any bug with the current code. > > Yep. I mention that in this part "This could potentially lead to an > overwrite of the objects following `pgmap` in `struct pci_p2pdma_pagemap`, > when `nr_range > 1`." Yes, but the commit message is not clear that nr_range can never be >1 in the code as it currently is. Logan
> > Yes, but the commit message is not clear that nr_range can never be >1 > in the code as it currently is. Done: https://lore.kernel.org/linux-hardening/ZRsUL%2FhATNruwtla@work/ Thanks! -- Gustavo
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index fa7370f9561a..ab34d3d36a64 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c @@ -28,9 +28,9 @@ struct pci_p2pdma { }; struct pci_p2pdma_pagemap { - struct dev_pagemap pgmap; struct pci_dev *provider; u64 bus_offset; + struct dev_pagemap pgmap; }; static struct pci_p2pdma_pagemap *to_p2p_pgmap(struct dev_pagemap *pgmap)
`struct dev_pagemap` is a flexible structure, which means that it contains a flexible-array member at the bottom. This could potentially lead to an overwrite of the objects following `pgmap` in `struct pci_p2pdma_pagemap`, when `nr_range > 1`. Fix this by placing the declaration of object `pgmap` at the end of `struct pci_p2pdma_pagemap`. -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting ready to enable it globally. Fixes: 0afea3814358 ("PCI/P2PDMA: Add provider's pci_dev to pci_p2pdma_pagemap struct") Fixes: a6e6fe6549f6 ("PCI/P2PDMA: Introduce private pagemap structure") Cc: stable@vger.kernel.org Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/pci/p2pdma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)