linux-kernel - Re: [PATCH v5 1/7] iommu: Fix a leak in iommu_insert_resv

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: <c4edcb80-4003-7a1c-fca1-cad25436a5ad@redhat.com>
Date:   Wed, 29 May 2019 17:38:15 +0200
From:   Auger Eric <eric.auger@...hat.com>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     eric.auger.pro@...il.com, joro@...tes.org,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        dwmw2@...radead.org, robin.murphy@....com,
        jean-philippe.brucker@....com, alex.williamson@...hat.com
Subject: Re: [PATCH v5 1/7] iommu: Fix a leak in iommu_insert_resv_region

Hi Christoph,

On 5/29/19 8:17 AM, Christoph Hellwig wrote:
>>  		} else if ((start >= a) && (end <= b)) {
>>  			if (new->type == type)
>> -				goto done;
>> +				return 0;
>>  			else
>>  				pos = pos->next;
> 
> Please remove the pointless else after the return statement.
Sorry I don't get your point here. For both cases, we only return if
both types match. Otherwise we continue parsing the list as there may be
a region of the same type as @new following the current interval that
may need to be merged with @new. Let's consider the insertion of those
resv regions:

add_resv(head, 0xa001000, 0xa001FFF, IOMMU_RESV_TYPE1);
head -> 0x000000000a001000 0x000000000a001fff type1

add_resv(head, 0xa003000, 0xa003FFF, IOMMU_RESV_TYPE1);
head ->
0x000000000a001000 0x000000000a001fff type1 ->
0x000000000a003000 0x000000000a003fff type1

add_resv(head, 0xa002000, 0xa004FFF, IOMMU_RESV_TYPE2);
head ->
0x000000000a001000 0x000000000a001fff type1 ->
0x000000000a003000 0x000000000a003fff type1 ->
0x000000000a002000 0x000000000a004fff type2 ->

add_resv(head, 0xa0010FF, 0xa003000, IOMMU_RESV_TYPE2);

head ->
0x000000000a001000 0x000000000a001fff type1 ->
0x000000000a003000 0x000000000a003fff type1 ->
0x000000000a0010ff 0x000000000a004fff type2 <- merge

Or maybe I simply missed your point. Please let me know.

Thanks

Eric

> 
>>  		} else {
>>  			if (new->type == type) {
>>  				phys_addr_t new_start = min(a, start);
>>  				phys_addr_t new_end = max(b, end);
>> +				int ret;
>>  
>>  				list_del(&entry->list);
>>  				entry->start = new_start;
>>  				entry->length = new_end - new_start + 1;
>> -				iommu_insert_resv_region(entry, regions);
>> +				ret = iommu_insert_resv_region(entry, regions);
>> +				kfree(entry);
>> +				return ret;
>>  			} else {
>>  				pos = pos->next;
>>  			}
> 
> Same here.
>