Re: [PATCH] arm64: kaslr: ensure randomized quantities are clean to the PoC

From: Catalin Marinas <catalin.marinas@arm.com>
To: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH] arm64: kaslr: ensure randomized quantities are clean to the PoC
Date: Wed, 16 Jan 2019 11:58:05 +0000	[thread overview]
Message-ID: <20190116115805.jarm4op4ydbkofbr@mbp> (raw)
In-Reply-To: <20190116113227.GB11240@fuggles.cambridge.arm.com>

On Wed, Jan 16, 2019 at 11:32:27AM +0000, Will Deacon wrote:
> On Tue, Jan 15, 2019 at 09:12:41PM +0100, Ard Biesheuvel wrote:
> > On Tue, 15 Jan 2019 at 20:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > kaslr_early_init() is called with the kernel mapped at its
> > > link time offset, and if it returns with a non-zero offset,
> > > the kernel is unmapped and remapped again at the randomized
> > > offset.
> > >
> > > During its execution, kaslr_early_init() also randomizes the
> > > base of the module region and of the linear mapping of DRAM,
> > > and sets two variables accordingly. However, since these
> > > variables are assigned with the caches on, they may get lost
> > > during the cache maintenance that occurs when unmapping and
> > > remapping the kernel, so ensure that these values are cleaned
> > > to the PoC.
> > >
> > > Fixes: f80fb3a3d508 ("arm64: add support for kernel ASLR")
> > > Cc: <stable@vger.kernel.org> # v4.6+
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > 
> > On an affected kernel build, I see the following variables sharing the
> > same 64-byte cacheline
> > 
> > ffff00001100bd18 R module_alloc_base
> > ffff00001100bd20 R memstart_addr
> > ffff00001100bd28 R arm64_dma_phys_limit
> > ffff00001100bd30 R kimage_voffset
> > ffff00001100bd38 R vabits_user
> > 
> > and we now have the following code in head.S (line 330)
> > 
> > adr_l x6, vabits_user
> > str x5, [x6]
> > dmb sy
> > dc ivac, x6 // Invalidate potentially stale cache line
> > 
> > which is invoked after kaslr_early_init() returns.
> 
> Urgh, this is hideous. Well done for debugging it! It would be nice to make
> this less fragile in future, perhaps by annotating the variable declarations
> and ensuring they're padded to the CWG etc so that we avoid any false
> sharing and make it very obvious what's going to be invalidated. We already
> have .mmuoff.data.write for data written with the MMU off.
> 
> I'm happy to take your patch to plug the regression, but I think we should
> aim to remove the false sharing rather than handling it. Another alternative
> might be to use clean+invalidate instead of invalidate in head.S, along the
> lines of:
> 
> 	DC CIVAC,	X0
> 	DMB		SY
> 	STR		X1, [X0]
> 	DMB		SY
> 	DC CIVAC,	X0
> 
> Can you foresee any issues with that?

The assumptions we've made in head.S were that any meaningful data has
already been written back by whoever wrote it (we don't care about clean
entries in the D-cache). Ard's patch fixes the code for this assumption.

I don't think changing head.S to CIVAC breaks anything since dirty lines
can be randomly evicted anyway. I'm more worried about no longer
detecting potential issues in things running prior to the kernel
(whether it's boot-loader, qemu).

FWIW, on Ard's patch:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

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