| 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
| ||
|
Message-ID: <615426dc-0ffd-9838-ade7-74eba12632d7@codeaurora.org>
Date: Fri, 12 Feb 2021 13:07:50 +0530
From: Akhil P Oommen <akhilpo@...eaurora.org>
To: linux-arm-msm@...r.kernel.org, Daniel Vetter <daniel@...ll.ch>,
David Airlie <airlied@...ux.ie>, Eric Anholt <eric@...olt.net>,
Jonathan Marek <jonathan@...ek.ca>,
Rob Clark <robdclark@...il.com>,
Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
Sean Paul <sean@...rly.run>,
Sharat Masetty <smasetty@...eaurora.org>,
dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm/msm: a6xx: Make sure the SQE microcode is safe
On 2/11/2021 9:32 PM, Jordan Crouse wrote:
> On Thu, Feb 11, 2021 at 06:50:28PM +0530, Akhil P Oommen wrote:
>> On 2/10/2021 6:22 AM, Jordan Crouse wrote:
>>> Most a6xx targets have security issues that were fixed with new versions
>>> of the microcode(s). Make sure that we are booting with a safe version of
>>> the microcode for the target and print a message and error if not.
>>>
>>> v2: Add more informative error messages and fix typos
>>>
>>> Signed-off-by: Jordan Crouse <jcrouse@...eaurora.org>
>>> ---
>>>
>>> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 77 ++++++++++++++++++++++-----
>>> 1 file changed, 64 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> index ba8e9d3cf0fe..064b7face504 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> @@ -522,28 +522,73 @@ static int a6xx_cp_init(struct msm_gpu *gpu)
>>> return a6xx_idle(gpu, ring) ? 0 : -EINVAL;
>>> }
>>> -static void a6xx_ucode_check_version(struct a6xx_gpu *a6xx_gpu,
>>> +/*
>>> + * Check that the microcode version is new enough to include several key
>>> + * security fixes. Return true if the ucode is safe.
>>> + */
>>> +static bool a6xx_ucode_check_version(struct a6xx_gpu *a6xx_gpu,
>>> struct drm_gem_object *obj)
>>> {
>>> + struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
>>> + struct msm_gpu *gpu = &adreno_gpu->base;
>>> u32 *buf = msm_gem_get_vaddr(obj);
>>> + bool ret = false;
>>> if (IS_ERR(buf))
>>> - return;
>>> + return false;
>>> /*
>>> - * If the lowest nibble is 0xa that is an indication that this microcode
>>> - * has been patched. The actual version is in dword [3] but we only care
>>> - * about the patchlevel which is the lowest nibble of dword [3]
>>> - *
>>> - * Otherwise check that the firmware is greater than or equal to 1.90
>>> - * which was the first version that had this fix built in
>>> + * Targets up to a640 (a618, a630 and a640) need to check for a
>>> + * microcode version that is patched to support the whereami opcode or
>>> + * one that is new enough to include it by default.
>>> */
>>> - if (((buf[0] & 0xf) == 0xa) && (buf[2] & 0xf) >= 1)
>>> - a6xx_gpu->has_whereami = true;
>>> - else if ((buf[0] & 0xfff) > 0x190)
>>> - a6xx_gpu->has_whereami = true;
>>> + if (adreno_is_a618(adreno_gpu) || adreno_is_a630(adreno_gpu) ||
>>> + adreno_is_a640(adreno_gpu)) {
nit: I feel a 'switch(revn)' would be more readable.
Reviewed-by: Akhil P Oommen <akhilpo@...eaurora.org>
-Akhil
>>> + /*
>>> + * If the lowest nibble is 0xa that is an indication that this
>>> + * microcode has been patched. The actual version is in dword
>>> + * [3] but we only care about the patchlevel which is the lowest
>>> + * nibble of dword [3]
>>> + *
>>> + * Otherwise check that the firmware is greater than or equal
>>> + * to 1.90 which was the first version that had this fix built
>>> + * in
>>> + */
>>> + if ((((buf[0] & 0xf) == 0xa) && (buf[2] & 0xf) >= 1) ||
>>> + (buf[0] & 0xfff) >= 0x190) {
>>> + a6xx_gpu->has_whereami = true;
>>> + ret = true;
>>> + goto out;
>>> + }
>>> + DRM_DEV_ERROR(&gpu->pdev->dev,
>>> + "a630 SQE ucode is too old. Have version %x need at least %x\n",
>>> + buf[0] & 0xfff, 0x190);
>>> + } else {
>>> + /*
>>> + * a650 tier targets don't need whereami but still need to be
>>> + * equal to or newer than 1.95 for other security fixes
>>> + */
>>> + if (adreno_is_a650(adreno_gpu)) {
>>> + if ((buf[0] & 0xfff) >= 0x195) {
>>> + ret = true;
>>> + goto out;
>>> + }
>>> +
>>> + DRM_DEV_ERROR(&gpu->pdev->dev,
>>> + "a650 SQE ucode is too old. Have version %x need at least %x\n",
>>> + buf[0] & 0xfff, 0x195);
>>> + }
>>> +
>>> + /*
>>> + * When a660 is added those targets should return true here
>>> + * since those have all the critical security fixes built in
>>> + * from the start
>>> + */
>> Or we can just initialize 'ret' as true.
>
> I thought about it and I think I want to force an accept list here instead of
> letting new targets get by with an implicit pass.
>
> Jordan
>
>> -Akhil
>>> + }
>>> +out:
>>> msm_gem_put_vaddr(obj);
>>> + return ret;
>>> }
>>> static int a6xx_ucode_init(struct msm_gpu *gpu)
>>> @@ -566,7 +611,13 @@ static int a6xx_ucode_init(struct msm_gpu *gpu)
>>> }
>>> msm_gem_object_set_name(a6xx_gpu->sqe_bo, "sqefw");
>>> - a6xx_ucode_check_version(a6xx_gpu, a6xx_gpu->sqe_bo);
>>> + if (!a6xx_ucode_check_version(a6xx_gpu, a6xx_gpu->sqe_bo)) {
>>> + msm_gem_unpin_iova(a6xx_gpu->sqe_bo, gpu->aspace);
>>> + drm_gem_object_put(a6xx_gpu->sqe_bo);
>>> +
>>> + a6xx_gpu->sqe_bo = NULL;
>>> + return -EPERM;
>>> + }
>>> }
>>> gpu_write64(gpu, REG_A6XX_CP_SQE_INSTR_BASE_LO,
>>>
>>
>
Powered by blists - more mailing lists