Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default

[Will Deacon <will.deacon@arm.com>]

On Thu, May 02, 2019 at 02:45:25PM +0200, Thierry Reding wrote:
> On Thu, May 02, 2019 at 12:08:21PM +0100, Will Deacon wrote:
> > On Thu, May 02, 2019 at 12:59:12PM +0200, Thierry Reding wrote:
> > > This made it to linux-next yesterday (less than a week before the merge
> > > window opens) and deliberately breaks existing configurations. That's a
> > > little rude.
> > > 
> > > At least give people a fair heads-up and a chance to fix things before
> > > you start break things.
> > 
> > Sorry about the inconvenience here.
> > 
> > This patch has been floating around for a while (albeit not in -next, since
> > I send my stuff via Joerg)
> 
> You can't expect people to test random patches from the list if they're
> not on Cc. I don't think it's safe to claim that patches have been well
> tested until they've been in linux-next for at least a couple of days.

I didn't claim it had been well tested. I also don't think being in
linux-next implies a patch has been well tested, to be honest with you.
What I can say is that this has been discussed on the public mailing list
for some time, and the outcome of that discussion is this patch.

> >                            and is heading for 5.3, so you have ages to fix
> > up your config!
> 
> Last I checked, Joerg applied this for 5.2 because you sent it as part
> of your "Updates for 5.2" pull request.

Sorry, I meant 5.2: the kernel that will be released in ~2 months time,
during which you'll be able to fix issues like this one. If we're having
unresolvable issues late in the cycle, then we can clearly revert this
patch.

> >                 It would, of course, be better to configure the IOMMU to
> > provide mappings for your DMA peripherals, but the trivial config change
> > will be enough to keep things working. We won't remove that as long as
> > people are relying on it.
> 
> I don't think the Kconfig option is really useful. People nowadays want
> to run standard distribution kernels on their devices, and distribution
> maintainers will often rely on kernel developers' guidance on what good
> defaults are. This patch suggests that the default should be to disable
> bypass, so if this hits 5.2 final and distributions create their kernel
> packages, they're likely going to go with this default and potentially
> break things for many of their users.

I'd say that people nowadays also want security by default, so it's a tough
crowd to please. You can still fix your .dts (I see there's a patch from Jon
doing that), or pass "arm-smmu.disable_bypass=0" on the cmdline to fix
things if you're not in a position to change the .config for your kernel
image.

> Luckily this seems like it's fairly easy to fix, but given that we're
> past v5.1-rc6, fixes for this now need to get special treatment. That
> would've been okay if this was a pressing issues, but this is changing
> something that's worked this way for ages, so it's hardly urgent.

Why the special treatment? You can continue to merge fixes after the merge
window, no? I feel like I'm missing something in your workflow here.

> > I don't expect most people to run into problems with this change (the new
> > behaviour matches what SMMUv3 does already).
> 
> I see the ARM SMMU v2 used in quite a few DTS files. Not all of these
> may be problematic, but I'd be somewhat surprised if Tegra was the only
> one impacted.

I didn't say this was specific to Tegra, only that I don't expect most
people to see any issue. I'm sure there will be others, and we can fix
them up as they appear. I can't think of a better way of toggling the
default behaviour.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel