Re: [PATCH 1/2] ACPI/IORT: Handle potential overflow in iort_dma_setup

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

On 10/01/2019 10:44, Auger Eric wrote:
> Hi Robin, Drew,
> 
> On 12/19/18 2:18 PM, Andrew Jones wrote:
>> On Wed, Dec 19, 2018 at 12:21:35PM +0000, Robin Murphy wrote:
>>> On 18/12/2018 18:48, Andrew Jones wrote:
>>>> The sum of dmaaddr and size may overflow, particularly considering
>>>> there are cases where size will be U64_MAX.
>>>
>>> Only if the firmware is broken in the first place, though. It would be weird
>>> to describe an explicit _DMA range of base=0 and size=U64_MAX, because it's
>>> effectively the same as just not having one at all, but it's not strictly
>>> illegal. However, since the ACPI System Memory address space is at most
>>> 64-bit, anything that would actually overflow here is already describing an
>>> impossibility - really, we should probably scream even louder about a
>>> firmware bug and reject it entirely, rather than quietly hiding it.
>>
>> Ah, looking again I see the paths. Either acpi_dma_get_range() returns
>> success, in which case base and size are fine, or it returns an EINVAL,
>> in which case base=size=0, or it returns ENODEV in which case base is
>> zero, so size may be set to U64_MAX by rc_dma_get_range() with no problem.
>> The !dev_is_pci(dev) path is also fine since base=0.
> 
> So practically putting an explicit memory_address_limit=64 is harmless
> as dmaaddr always is 0, right?
> 
> In QEMU I intended to update the ACPI code to comply with the rev D
> spec. in that case the RC table revision is 1 (rev D) and the
> memory_address_limit needs to be filled. If we don't want to restrict
> the limit, isn't it the right choice to set 64 here?

Indeed, the Memory Address Size Limit doesn't cater for offsets so can't 
run into this kind of overflow in the first place. For a fully-emulated 
PCI hierarchy I'd say 64 is not just harmless but in fact entirely 
correct - you're going to have more fun with VFIO passthrough if the 
host tables have more restrictive limits, but I guess that's a problem 
for the future ;)

Robin.