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

Message-ID: <69ad6ee1-60d5-61e1-8c07-fc81e9425722@canonical.com>
Date:   Sat, 14 Nov 2020 22:09:55 +0000
From:   Colin Ian King <colin.king@...onical.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
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

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@...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",
> 

Thanks.