* [PATCH][V2] PCI: Fix a potential uninitentional integer overflow issue @ 2020-11-10 22:10 Colin King 2020-11-10 23:55 ` Logan Gunthorpe 2020-11-14 21:53 ` Bjorn Helgaas 0 siblings, 2 replies; 4+ messages in thread From: Colin King @ 2020-11-10 22:10 UTC (permalink / raw) To: Bjorn Helgaas, Stephen Bates, Alex Williamson, Christian König, Logan Gunthorpe, linux-pci Cc: kernel-janitors, linux-kernel From: Colin Ian King <colin.king@canonical.com> The shift of 1 by align_order is evaluated using 32 bit arithmetic and the result is assigned to a resource_size_t type variable that is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow before widening issue by making the 1 a ULL. Addresses-Coverity: ("Unintentional integer overflow") Fixes: 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters reusable") Signed-off-by: Colin Ian King <colin.king@canonical.com> --- V2: Use ULL instead of BIT_ULL(), fix spelling mistake and capitalize first word of patch subject. --- drivers/pci/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 3ef63a101fa1..248044a7ef8c 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -6214,7 +6214,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev, if (align_order == -1) align = PAGE_SIZE; else - align = 1 << align_order; + align = 1ULL << align_order; break; } else if (ret < 0) { pr_err("PCI: Can't parse resource_alignment parameter: %s\n", -- 2.28.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][V2] PCI: Fix a potential uninitentional integer overflow issue 2020-11-10 22:10 [PATCH][V2] PCI: Fix a potential uninitentional integer overflow issue Colin King @ 2020-11-10 23:55 ` Logan Gunthorpe 2020-11-14 21:53 ` Bjorn Helgaas 1 sibling, 0 replies; 4+ messages in thread From: Logan Gunthorpe @ 2020-11-10 23:55 UTC (permalink / raw) To: Colin King, Bjorn Helgaas, Stephen Bates, Alex Williamson, Christian König, linux-pci Cc: kernel-janitors, linux-kernel On 2020-11-10 3:10 p.m., Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > The shift of 1 by align_order is evaluated using 32 bit arithmetic > and the result is assigned to a resource_size_t type variable that > is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow > before widening issue by making the 1 a ULL. > > Addresses-Coverity: ("Unintentional integer overflow") > Fixes: 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters reusable") I think this should probably be Fixes: 32a9a682bef2 ("PCI: allow assignment of memory resources with a specified alignment") That is the commit where the original bug was introduced. 644a544fd9bcd then extends the code a little bit and 07d8d7e57c28 only refactors it into a reusable function. If we want this in older stable kernels then we will probably need to make different patches for the other two vintages. > Signed-off-by: Colin Ian King <colin.king@canonical.com> Besides that, the change makes sense to me. Reviewed-by: Logan Gunthorpe <logang@deltatee.com> Thanks, Logan ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][V2] PCI: Fix a potential uninitentional integer overflow issue 2020-11-10 22:10 [PATCH][V2] PCI: Fix a potential uninitentional integer overflow issue Colin King 2020-11-10 23:55 ` Logan Gunthorpe @ 2020-11-14 21:53 ` Bjorn Helgaas 2020-11-14 22:09 ` Colin Ian King 1 sibling, 1 reply; 4+ messages in thread From: Bjorn Helgaas @ 2020-11-14 21:53 UTC (permalink / raw) To: Colin King Cc: Bjorn Helgaas, Stephen Bates, Alex Williamson, Christian König, Logan Gunthorpe, linux-pci, kernel-janitors, linux-kernel, Dan Carpenter [+cc Dan] On Tue, Nov 10, 2020 at 10:10:48PM +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > The shift of 1 by align_order is evaluated using 32 bit arithmetic > and the result is assigned to a resource_size_t type variable that > is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow > before widening issue by making the 1 a ULL. > > Addresses-Coverity: ("Unintentional integer overflow") > Fixes: 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters reusable") > Signed-off-by: Colin Ian King <colin.king@canonical.com> Applied to pci/misc for v5.11 with Logan's Reviewed-by and also the Fixes: correction. I first applied the patch below to bounds-check the alignment as noted by Dan. > --- > > V2: Use ULL instead of BIT_ULL(), fix spelling mistake and capitalize first > word of patch subject. > > --- > drivers/pci/pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 3ef63a101fa1..248044a7ef8c 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -6214,7 +6214,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev, > if (align_order == -1) > align = PAGE_SIZE; > else > - align = 1 << align_order; > + align = 1ULL << align_order; > break; > } else if (ret < 0) { > pr_err("PCI: Can't parse resource_alignment parameter: %s\n", commit d6ca242c448f ("PCI: Bounds-check command-line resource alignment requests") Author: Bjorn Helgaas <bhelgaas@google.com> Date: Thu Nov 5 14:51:36 2020 -0600 PCI: Bounds-check command-line resource alignment requests 32-bit BARs are limited to 2GB size (2^31). By extension, I assume 64-bit BARs are limited to 2^63 bytes. Limit the alignment requested by the "pci=resource_alignment=" command-line parameter to 2^63. Link: https://lore.kernel.org/r/20201007123045.GS4282@kadam Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 8b9bea8ba751..26c1b2d0bacd 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -6197,19 +6197,21 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev, while (*p) { count = 0; if (sscanf(p, "%d%n", &align_order, &count) == 1 && - p[count] == '@') { + p[count] == '@') { p += count + 1; + if (align_order > 63) { + pr_err("PCI: Invalid requested alignment (order %d)\n", + align_order); + align_order = PAGE_SHIFT; + } } else { - align_order = -1; + align_order = PAGE_SHIFT; } ret = pci_dev_str_match(dev, p, &p); if (ret == 1) { *resize = true; - if (align_order == -1) - align = PAGE_SIZE; - else - align = 1 << align_order; + align = 1 << align_order; break; } else if (ret < 0) { pr_err("PCI: Can't parse resource_alignment parameter: %s\n", ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][V2] PCI: Fix a potential uninitentional integer overflow issue 2020-11-14 21:53 ` Bjorn Helgaas @ 2020-11-14 22:09 ` Colin Ian King 0 siblings, 0 replies; 4+ messages in thread From: Colin Ian King @ 2020-11-14 22:09 UTC (permalink / raw) To: Bjorn Helgaas Cc: Bjorn Helgaas, Stephen Bates, Alex Williamson, Christian König, Logan Gunthorpe, linux-pci, kernel-janitors, linux-kernel, Dan Carpenter On 14/11/2020 21:53, Bjorn Helgaas wrote: > [+cc Dan] > > On Tue, Nov 10, 2020 at 10:10:48PM +0000, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> The shift of 1 by align_order is evaluated using 32 bit arithmetic >> and the result is assigned to a resource_size_t type variable that >> is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow >> before widening issue by making the 1 a ULL. >> >> Addresses-Coverity: ("Unintentional integer overflow") >> Fixes: 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters reusable") >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > > Applied to pci/misc for v5.11 with Logan's Reviewed-by and also the > Fixes: correction. > > I first applied the patch below to bounds-check the alignment as noted > by Dan. > >> --- >> >> V2: Use ULL instead of BIT_ULL(), fix spelling mistake and capitalize first >> word of patch subject. >> >> --- >> drivers/pci/pci.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 3ef63a101fa1..248044a7ef8c 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -6214,7 +6214,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev, >> if (align_order == -1) >> align = PAGE_SIZE; >> else >> - align = 1 << align_order; >> + align = 1ULL << align_order; >> break; >> } else if (ret < 0) { >> pr_err("PCI: Can't parse resource_alignment parameter: %s\n", > > commit d6ca242c448f ("PCI: Bounds-check command-line resource alignment requests") > Author: Bjorn Helgaas <bhelgaas@google.com> > Date: Thu Nov 5 14:51:36 2020 -0600 > > PCI: Bounds-check command-line resource alignment requests > > 32-bit BARs are limited to 2GB size (2^31). By extension, I assume 64-bit > BARs are limited to 2^63 bytes. Limit the alignment requested by the > "pci=resource_alignment=" command-line parameter to 2^63. > > Link: https://lore.kernel.org/r/20201007123045.GS4282@kadam > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 8b9bea8ba751..26c1b2d0bacd 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -6197,19 +6197,21 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev, > while (*p) { > count = 0; > if (sscanf(p, "%d%n", &align_order, &count) == 1 && > - p[count] == '@') { > + p[count] == '@') { > p += count + 1; > + if (align_order > 63) { > + pr_err("PCI: Invalid requested alignment (order %d)\n", > + align_order); > + align_order = PAGE_SHIFT; > + } > } else { > - align_order = -1; > + align_order = PAGE_SHIFT; > } > > ret = pci_dev_str_match(dev, p, &p); > if (ret == 1) { > *resize = true; > - if (align_order == -1) > - align = PAGE_SIZE; > - else > - align = 1 << align_order; > + align = 1 << align_order; > break; > } else if (ret < 0) { > pr_err("PCI: Can't parse resource_alignment parameter: %s\n", > Thanks. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-14 22:10 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-10 22:10 [PATCH][V2] PCI: Fix a potential uninitentional integer overflow issue Colin King 2020-11-10 23:55 ` Logan Gunthorpe 2020-11-14 21:53 ` Bjorn Helgaas 2020-11-14 22:09 ` Colin Ian King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).