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

[Andrew Jones <drjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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.

While I agree that we should complain when firmware provides bad ACPI
tables, my understanding of setting iort.memory_address_limit=64 was
that it simply meant "no limit", regardless of the base. However I now
see that it won't be used unless base=0. So I don't think we have a
problem, and we don't even seem to be missing firmware sanity checks.

It might be nice to have a comment explaining this stuff somewhere, but
otherwise sorry for the noise.

Thanks,
drew

> 
> Robin.
> 
> > Signed-off-by: Andrew Jones <drjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >   drivers/acpi/arm64/iort.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index 70f4e80b9246..a0f4c157ba5e 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -1002,7 +1002,12 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
> >   	}
> >   	if (!ret) {
> > -		msb = fls64(dmaaddr + size - 1);
> > +		u64 dmaaddr_max = dmaaddr + size - 1;
> > +		if (dmaaddr_max >= dmaaddr)
> > +			msb = fls64(dmaaddr_max);
> > +		else
> > +			msb = 64;
> > +
> >   		/*
> >   		 * Round-up to the power-of-two mask or set
> >   		 * the mask to the whole 64-bit address space
> >