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 <[email protected]>
---
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)) {
+ /*
+ * 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
+ */
+ }
+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,
--
2.25.1
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 <[email protected]>
> ---
>
> 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)) {
> + /*
> + * 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.
-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,
>
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 <[email protected]>
> >---
> >
> > 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)) {
> >+ /*
> >+ * 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,
> >
>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
>>> ---
>>>
>>> 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 <[email protected]>
-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,
>>>
>>
>
Hello,
On 10/02/2021 03:52, 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 <[email protected]>
[skipped]
> + } 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;
> + }
I think this is incorrect. The latest firmware i have here also fails
this check, with the buf[0] = 0x016dd099, so buf[0] & 0xfff = 0x099.
Could you please confirm the versioning?
> +
> + 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
> + */
> + }
> +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,
--
With best wishes
Dmitry