linux-hardening - Re: [PATCH][next] iommu/vt-d: Fix out-bounds-warning in intel_svm_page

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 14 Apr 2021 15:04:24 -0500
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     Lu Baolu <baolu.lu@...ux.intel.com>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>
Cc:     iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] iommu/vt-d: Fix out-bounds-warning in
 intel_svm_page_response()

Hi Balou,

On 4/14/21 00:24, Lu Baolu wrote:
> Hi Gustavo,
> 
> On 4/14/21 3:54 AM, Gustavo A. R. Silva wrote:
>> Replace call to memcpy() with just a couple of simple assignments in
>> order to fix the following out-of-bounds warning:
>>
>> drivers/iommu/intel/svm.c:1198:4: warning: 'memcpy' offset [25, 32] from the object at 'desc' is out of the bounds of referenced subobject 'qw2' with type
>> 'long long unsigned int' at offset 16 [-Warray-bounds]
>>
>> The problem is that the original code is trying to copy data into a
>> couple of struct members adjacent to each other in a single call to
>> memcpy(). This causes a legitimate compiler warning because memcpy()
>> overruns the length of &desc.qw2.
>>
>> This helps with the ongoing efforts to globally enable -Warray-bounds
>> and get us closer to being able to tighten the FORTIFY_SOURCE routines
>> on memcpy().
>>
>> Link: https://github.com/KSPP/linux/issues/109
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
>> ---
>>   drivers/iommu/intel/svm.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>> index 5165cea90421..65909f504c50 100644
>> --- a/drivers/iommu/intel/svm.c
>> +++ b/drivers/iommu/intel/svm.c
>> @@ -1194,9 +1194,10 @@ int intel_svm_page_response(struct device *dev,
>>           desc.qw1 = QI_PGRP_IDX(prm->grpid) | QI_PGRP_LPIG(last_page);
>>           desc.qw2 = 0;
>>           desc.qw3 = 0;
>> -        if (private_present)
>> -            memcpy(&desc.qw2, prm->private_data,
>> -                   sizeof(prm->private_data));
> 
> The same memcpy() is used in multiple places in this file. Did they
> compile the same warnings? Or there are multiple patches to fix them
> one by one?

I just see one more instance of this same case:

1023                         if (req->priv_data_present)
1024                                 memcpy(&resp.qw2, req->priv_data,
1025                                        sizeof(req->priv_data));

I missed it and I'll address it in v2. Do you see another one?

Thanks for the feedback!
--
Gustavo