Re: [PATCH][next] iommu/vt-d: Fix out-bounds-warning in intel_svm_page_response()

[Gustavo A. R. Silva]

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@xxxxxxxxxx>
>> ---
>>   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