linux-kernel - Re: [PATCH][V2] PCI: Fix a potential uninitentional integer overflow issue

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

Message-ID: <20201114215329.GA1197070@bjorn-Precision-5520>
Date:   Sat, 14 Nov 2020 15:53:29 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Colin King <colin.king@...onical.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Stephen Bates <sbates@...thlin.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Christian König <christian.koenig@....com>,
        Logan Gunthorpe <logang@...tatee.com>,
        linux-pci@...r.kernel.org, kernel-janitors@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH][V2] PCI: Fix a potential uninitentional integer overflow
 issue

[+cc Dan]

On Tue, Nov 10, 2020 at 10:10:48PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@...onical.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@...onical.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@...gle.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@...cle.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.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",