Re: [PATCH for-next/fixes] arm64/mm: Fix false-positive !virt_addr_valid() for kernel image

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Nov 25, 2024 at 03:49:10PM +0100, Lukas Wunner wrote:
> On Mon, Nov 25, 2024 at 10:50:48AM +0000, Mark Rutland wrote:
> > On Mon, Nov 25, 2024 at 10:54:49AM +0100, Lukas Wunner wrote:
> > > Other arches do not seem to be concerned about this and
> > > let virt_addr_valid() return true for the kernel image.
> > > It's not clear why arm64 is special and needs to return false.
> > > 
> > > However, I agree there's hardly ever a reason to DMA from/to the
> > > .text section.  From a security perspective, constraining this to
> > > .rodata seems reasonable to me and I'll be happy to amend the patch
> > > to that effect if that's the consensus.
> > 
> > Instead, can we update the test to use lm_alias() on the symbols in
> > question? That'll convert a kernel image address to its linear map
> > alias, and then that'll work with virt_addr_valid(), virt_to_phys(),
> > etc.
> 
> Do you mean that sg_set_buf() should pass the address to lm_alias()
> if it points into the kernel image?

No; I meant that the test could use lm_alias() on the test vectors
before passing those to sg_set_buf(), when the test code knows by
construction that those vectors happen to be part of the kernel image.
That'll work on all architectures.

That said, looking at the code it appears that testmgr.c can be built as
a module, so the test vectors could be module/vmalloc addresses rather
than virt/linear or image addresses. Given that, I don't think the
changes suggested here are sufficient, as module addresses should still
be rejected.

Can we kmemdup() the test vector data first? That'd give us a legitimate
virt/linear address that we can use.

> That would require a helper to determine whether it's a kernel image
> address or not.  It seems we do not have such a cross-architecture
> helper (but maybe I'm missing something).  (I am adding an arm64-specific
> one in the proposed patch.)
> 
> So this doesn't look like a viable approach.
> 
> Also, I'd expect pushback against an sg_set_buf() change which is
> only necessary to accommodate arm64.  I'd expect the obvious question
> to be asked, which is why arm64's virt_addr_valid() can't behave like
> any other architecture's.  And honestly I wouldn't know what to answer.

I don't think arm64 is doing anything wrong here; an image address is
*not* a virt/linear address, and we only fix that up in virt_to_*() as a
best-effort thing. I think other architectures which accept that are
signing themselves up for subtle bugs that we're trying to prevent
early.

Mark.