linux-kernel - Re: [PATCH] drm/msm/a6xx: Skip gpu secure fw load in EL2 mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b71d89f9-fb72-405a-b5bf-a09a796561bb@quicinc.com>
Date: Fri, 13 Dec 2024 08:35:27 +0530
From: Pavan Kondeti <quic_pkondeti@...cinc.com>
To: Mark Rutland <mark.rutland@....com>
CC: Marc Zyngier <maz@...nel.org>, Pavan Kondeti <quic_pkondeti@...cinc.com>,
        Akhil P Oommen <quic_akhilpo@...cinc.com>,
        Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
        Konrad Dybcio <konradybcio@...nel.org>,
        "Abhinav
 Kumar" <quic_abhinavk@...cinc.com>,
        Dmitry Baryshkov
	<dmitry.baryshkov@...aro.org>,
        Marijn Suijten
	<marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>, "Simona
 Vetter" <simona@...ll.ch>,
        Elliot Berman <quic_eberman@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        <freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] drm/msm/a6xx: Skip gpu secure fw load in EL2 mode

On Thu, Dec 12, 2024 at 10:40:46AM +0000, Mark Rutland wrote:
> On Thu, Dec 12, 2024 at 08:50:12AM +0000, Marc Zyngier wrote:
> > On Thu, 12 Dec 2024 05:31:00 +0000,
> > Pavan Kondeti <quic_pkondeti@...cinc.com> wrote:
> > > 
> > > On Wed, Dec 11, 2024 at 10:40:02AM +0000, Marc Zyngier wrote:
> > > > On Wed, 11 Dec 2024 00:37:34 +0000,
> > > > Pavan Kondeti <quic_pkondeti@...cinc.com> wrote:
> > > > > 
> > > > > On Tue, Dec 10, 2024 at 09:24:03PM +0000, Marc Zyngier wrote:
> > > > > > > +static int a6xx_switch_secure_mode(struct msm_gpu *gpu)
> > > > > > > +{
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +#ifdef CONFIG_ARM64
> > > > > > > +	/*
> > > > > > > +	 * We can access SECVID_TRUST_CNTL register when kernel is booted in EL2 mode. So, use it
> > > > > > > +	 * to switch the secure mode to avoid the dependency on zap shader.
> > > > > > > +	 */
> > > > > > > +	if (is_kernel_in_hyp_mode())
> > > > > > > +		goto direct_switch;
> > > > > > 
> > > > > > No, please. To check whether you are *booted* at EL2, you need to
> > > > > > check for is_hyp_available(). Whether the kernel runs at EL1 or EL2 is
> > > > > > none of the driver's business, really. This is still absolutely
> > > > > > disgusting from an abstraction perspective, but I guess we don't have
> > > > > > much choice here.
> > > > > > 
> > > > > 
> > > > > Thanks Marc. Any suggestions on how we can make is_hyp_mode_available()
> > > > > available for modules? Do you prefer exporting
> > > > > kvm_protected_mode_initialized and __boot_cpu_mode symbols directly or
> > > > > try something like [1]?
> > > > 
> > > > Ideally, neither. These were bad ideas nine years ago, and they still
> > > > are. The least ugly hack I can come up with is the patch below, and
> > > > you'd write something like:
> > > > 
> > > > 	if (cpus_have_cap(ARM64_HAS_EL2_OWNERSHIP))
> > > > 		blah();
> > > > 
> > > > This is obviously completely untested.
> > > > 
> > > 
> > > I have tested your patch. It works as intended. Thanks Marc.
> > 
> > Note that you will probably get some push-back from the arm64
> > maintainers on this front, because this is a fairly incomplete (and
> > fragile) solution.
> > 
> > It would be much better if the discriminant came from the device tree.
> > After all, the hypervisor is fscking-up^W^Wchanging the programming
> > model of the GPU, and that should be reflected in the DT. Because for
> > all intent and purposes, this is not the same hardware anymore.
> 
> FWIW I agree 100%, this should be described in DT.
> 
> The cpucap doesn't describe the actual property we care about, and it
> cannot in general (e.g. for nested virt). I would strongly prefer to not
> have that as it's setting ourselves up for failure.
> 
Thanks for the feedback. I understand that EL2 detection in kernel is
not going to cover cases like bare metal EL1, nested virtualization. We
plan to take the DT approach. 

Thanks,
Pavan