soc: fsl: qe: Replace one-element array and use struct_size() helper

The current codebase makes use of one-element arrays in the following form:

struct something {
    int length;
    u8 data[1];
};

struct something *instance;

instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL); instance->length = size;
memcpy(instance->data, source, size);

but the preferred mechanism to declare variable-length types such as these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being inadvertently introduced[3] to the codebase from now on. So, replace the one-element array with a flexible-array member.

Also, make use of the new struct_size() helper to properly calculate the size of struct qe_firmware.

This issue was found with the help of Coccinelle and, audited and fixed _manually_.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/soc/fsl/qe/qe.c | 4 ++--
 include/soc/fsl/qe/qe.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/soc/fsl/qe/qe.c +2 -2

index 447146861c2c1..2df20d6f85fa4 100644
@@ -448,7 +448,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
 	unsigned int i;
 	unsigned int j;
 	u32 crc;
-	size_t calc_size = sizeof(struct qe_firmware);
+	size_t calc_size;
 	size_t length;
 	const struct qe_header *hdr;
 

@@ -480,7 +480,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
 	}
 
 	/* Validate the length and check if there's a CRC */
-	calc_size += (firmware->count - 1) * sizeof(struct qe_microcode);
+	calc_size = struct_size(firmware, microcode, firmware->count);
 
 	for (i = 0; i < firmware->count; i++)
 		/*

include/soc/fsl/qe/qe.h +1 -1

index e282ac01ec081..3feddfec9f87d 100644
@@ -307,7 +307,7 @@ struct qe_firmware {
 		u8 revision;		/* The microcode version revision */
 		u8 padding;		/* Reserved, for alignment */
 		u8 reserved[4];		/* Reserved, for future expansion */
-	} __attribute__ ((packed)) microcode[1];
+	} __packed microcode[];
 	/* All microcode binaries should be located here */
 	/* CRC32 should be located here, after the microcode binaries */
 } __attribute__ ((packed));

Signature · 1 line

2.26.2

-----Original Message-----
From: Gustavo A. R. Silva <[email protected]>
Sent: 2020年5月19日 6:19
To: Qiang Zhao <[email protected]>; Leo Li <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Gustavo A. R. Silva <[email protected]>; Kees Cook <[email protected]>
Subject: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper

The current codebase makes use of one-element arrays in the following form:

struct something {
    int length;
    u8 data[1];
};

struct something *instance;

instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL); instance->length = size;
memcpy(instance->data, source, size);

but the preferred mechanism to declare variable-length types such as these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being inadvertently introduced[3] to the codebase from now on. So, replace the one-element array with a flexible-array member.

Also, make use of the new struct_size() helper to properly calculate the size of struct qe_firmware.

This issue was found with the help of Coccinelle and, audited and fixed _manually_.

[1]
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.
org%2Fonlinedocs%2Fgcc%2FZero-Length.html&amp;data=02%7C01%7Cqiang. zhao%40nxp.com%7Cb058bcb9af134df8446808d7fb78cfa9%7C686ea1d3bc2b 4c6fa92cd99c5c301635%7C0%7C0%7C637254368610203908&amp;sdata=70 a6VBg3oWQf9a5KICuCEuIj6gw57NKYhNv2JL8JdDY%3D&amp;reserved=0
[2]
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
om%2FKSPP%2Flinux%2Fissues%2F21&amp;data=02%7C01%7Cqiang.zhao%40 nxp.com%7Cb058bcb9af134df8446808d7fb78cfa9%7C686ea1d3bc2b4c6fa92c d99c5c301635%7C0%7C0%7C637254368610213901&amp;sdata=kVyNBYHbm a5jCO9pbkoHMetHEYGvWDq6Xw%2BBeC7uHII%3D&amp;reserved=0
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/soc/fsl/qe/qe.c | 4 ++--
 include/soc/fsl/qe/qe.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/soc/fsl/qe/qe.c index renamed from drivers/soc/fsl/qe/qe.c +0 -0

447146861c2c1..2df20d6f85fa4 100644

drivers/soc/fsl/qe/qe.c +2 -2

@@ -448,7 +448,7 @@ int qe_upload_firmware(const struct qe_firmware
 *firmware)
 	unsigned int i;
 	unsigned int j;
 	u32 crc;
-	size_t calc_size = sizeof(struct qe_firmware);
+	size_t calc_size;
 	size_t length;
 	const struct qe_header *hdr;

@@ -480,7 +480,7 @@ int qe_upload_firmware(const struct qe_firmware
 *firmware)
 	}
 
 	/* Validate the length and check if there's a CRC */
-	calc_size += (firmware->count - 1) * sizeof(struct qe_microcode);
+	calc_size = struct_size(firmware, microcode, firmware->count);
 
 	for (i = 0; i < firmware->count; i++)

drivers/soc/fsl/qe/qe.c +0 -0

 		/*

include/soc/fsl/qe/qe.h index renamed from include/soc/fsl/qe/qe.h +0 -0

e282ac01ec081..3feddfec9f87d 100644

include/soc/fsl/qe/qe.h +1 -2

@@ -307,7 +307,7 @@ struct qe_firmware {
 		u8 revision;		/* The microcode version revision */
 		u8 padding;		/* Reserved, for alignment */
 		u8 reserved[4];		/* Reserved, for future expansion */
-	} __attribute__ ((packed)) microcode[1];
+	} __packed microcode[];
 	/* All microcode binaries should be located here */
 	/* CRC32 should be located here, after the microcode binaries */  }
 __attribute__ ((packed));
--

2.26.2

Quoted 80 lines · Gustavo A. R. Silva

The current codebase makes use of one-element arrays in the following form:

struct something {
    int length;
    u8 data[1];
};

struct something *instance;

instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL); instance->length = size;
memcpy(instance->data, source, size);

but the preferred mechanism to declare variable-length types such as these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being inadvertently introduced[3] to the codebase from now on. So, replace the one-element array with a flexible-array member.

Also, make use of the new struct_size() helper to properly calculate the size of struct qe_firmware.

This issue was found with the help of Coccinelle and, audited and fixed _manually_.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/soc/fsl/qe/qe.c | 4 ++--
 include/soc/fsl/qe/qe.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/soc/fsl/qe/qe.c +2 -2

index 447146861c2c1..2df20d6f85fa4 100644
@@ -448,7 +448,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
      unsigned int i;
      unsigned int j;
      u32 crc;
-     size_t calc_size = sizeof(struct qe_firmware);
+     size_t calc_size;
      size_t length;
      const struct qe_header *hdr;
 

@@ -480,7 +480,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
      }
 
      /* Validate the length and check if there's a CRC */
-     calc_size += (firmware->count - 1) * sizeof(struct qe_microcode);
+     calc_size = struct_size(firmware, microcode, firmware->count);
 
      for (i = 0; i < firmware->count; i++)
              /*

include/soc/fsl/qe/qe.h +1 -2

index e282ac01ec081..3feddfec9f87d 100644
@@ -307,7 +307,7 @@ struct qe_firmware {
              u8 revision;            /* The microcode version revision */
              u8 padding;             /* Reserved, for alignment */
              u8 reserved[4];         /* Reserved, for future expansion */
-     } __attribute__ ((packed)) microcode[1];
+     } __packed microcode[];
      /* All microcode binaries should be located here */
      /* CRC32 should be located here, after the microcode binaries */
 } __attribute__ ((packed));
--

Quoted 2 lines

2.26.2

Hm, looking at this code, I see a few other things that need to be fixed:

1) drivers/tty/serial/ucc_uart.c does not do a be32_to_cpu() conversion on the length test (understandably, a little-endian system has never run this code since it's ppc specific), but it's still wrong:

        if (firmware->header.length != fw->size) {

compare to the firmware loader:

        length = be32_to_cpu(hdr->length);

2) drivers/soc/fsl/qe/qe.c does not perform bounds checking on the per-microcode offsets, so the uploader might send data outside the firmware buffer. Perhaps:

drivers/soc/fsl/qe/qe.c +23 -2

index 447146861c2c..c4e0bc452f03 100644
@@ -451,6 +451,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
        size_t calc_size = sizeof(struct qe_firmware);
        size_t length;
        const struct qe_header *hdr;
+       void *firmware_end;
 
        if (!firmware) {
                printk(KERN_ERR "qe-firmware: invalid pointer\n");

@@ -491,19 +492,39 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
                calc_size += sizeof(__be32) *
                        be32_to_cpu(firmware->microcode[i].count);
 
-       /* Validate the length */
+       /* Validate total length */
        if (length != calc_size + sizeof(__be32)) {
                printk(KERN_ERR "qe-firmware: invalid length\n");
                return -EPERM;
        }
 
        /* Validate the CRC */
-       crc = be32_to_cpu(*(__be32 *)((void *)firmware + calc_size));
+       firmware_end = (void *)firmware + calc_size;
+       crc = be32_to_cpu(*(__be32 *)firmware_end);
        if (crc != crc32(0, firmware, calc_size)) {
                printk(KERN_ERR "qe-firmware: firmware CRC is invalid\n");
                return -EIO;
        }
 
+       /* Validate ucode lengths and offsets */
+       for (i = 0; i < firmware->count; i++) {
+               const struct qe_microcode *ucode = &firmware->microcode[i];
+               __be32 *code;
+               size_t count;
+
+               if (!ucode->code_offset)
+                       continue;
+
+               code = (void *)firmware + be32_to_cpu(ucode->code_offset);
+               count = be32_to_cpu(ucode->count) * sizeof(*code);
+
+               if (code < firmware || code >= firmware_end ||
+                   code + count < firmware || code + count >= firmware_end) {
+                       printk(KERN_ERR "qe-firmware: invalid ucode offset\n");
+                       return -EIO;
+               }
+       }
+
        /*
         * If the microcode calls for it, split the I-RAM.
         */

I haven't tested this.

--
Kees Cook

Quoted 94 lines · Kees Cook

Quoted 80 lines · Gustavo A. R. Silva

The current codebase makes use of one-element arrays in the following form:

struct something {
    int length;
    u8 data[1];
};

struct something *instance;

instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL); instance->length = size;
memcpy(instance->data, source, size);

but the preferred mechanism to declare variable-length types such as these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being inadvertently introduced[3] to the codebase from now on. So, replace the one-element array with a flexible-array member.

Also, make use of the new struct_size() helper to properly calculate the size of struct qe_firmware.

This issue was found with the help of Coccinelle and, audited and fixed _manually_.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/soc/fsl/qe/qe.c | 4 ++--
 include/soc/fsl/qe/qe.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/soc/fsl/qe/qe.c +2 -2

index 447146861c2c1..2df20d6f85fa4 100644
@@ -448,7 +448,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
      unsigned int i;
      unsigned int j;
      u32 crc;
-     size_t calc_size = sizeof(struct qe_firmware);
+     size_t calc_size;
      size_t length;
      const struct qe_header *hdr;
 

@@ -480,7 +480,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
      }
 
      /* Validate the length and check if there's a CRC */
-     calc_size += (firmware->count - 1) * sizeof(struct qe_microcode);
+     calc_size = struct_size(firmware, microcode, firmware->count);
 
      for (i = 0; i < firmware->count; i++)
              /*

include/soc/fsl/qe/qe.h +1 -2

index e282ac01ec081..3feddfec9f87d 100644
@@ -307,7 +307,7 @@ struct qe_firmware {
              u8 revision;            /* The microcode version revision */
              u8 padding;             /* Reserved, for alignment */
              u8 reserved[4];         /* Reserved, for future expansion */
-     } __attribute__ ((packed)) microcode[1];
+     } __packed microcode[];
      /* All microcode binaries should be located here */
      /* CRC32 should be located here, after the microcode binaries */
 } __attribute__ ((packed));
--

Quoted 2 lines

2.26.2

Hm, looking at this code, I see a few other things that need to be fixed:

1) drivers/tty/serial/ucc_uart.c does not do a be32_to_cpu() conversion on the length test (understandably, a little-endian system has never run this code since it's ppc specific), but it's still wrong:

        if (firmware->header.length != fw->size) {

Quoted 7 lines

compare to the firmware loader:

        length = be32_to_cpu(hdr->length);

2) drivers/soc/fsl/qe/qe.c does not perform bounds checking on the per-microcode offsets, so the uploader might send data outside the firmware buffer. Perhaps:

We do validate the CRC for each microcode, it is unlikely the CRC check can pass if the offset or length is not correct. But you are probably right that it will be safer to check the boundary and fail quicker before we actually start the CRC check. Will you come up with a formal patch or you want us to deal with it?

Quoted 63 lines

drivers/soc/fsl/qe/qe.c +23 -2

index 447146861c2c..c4e0bc452f03 100644
@@ -451,6 +451,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
        size_t calc_size = sizeof(struct qe_firmware);
        size_t length;
        const struct qe_header *hdr;
+       void *firmware_end;
 
        if (!firmware) {
                printk(KERN_ERR "qe-firmware: invalid pointer\n");

@@ -491,19 +492,39 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
                calc_size += sizeof(__be32) *
                        be32_to_cpu(firmware->microcode[i].count);
 
-       /* Validate the length */
+       /* Validate total length */
        if (length != calc_size + sizeof(__be32)) {
                printk(KERN_ERR "qe-firmware: invalid length\n");
                return -EPERM;
        }
 
        /* Validate the CRC */
-       crc = be32_to_cpu(*(__be32 *)((void *)firmware + calc_size));
+       firmware_end = (void *)firmware + calc_size;
+       crc = be32_to_cpu(*(__be32 *)firmware_end);
        if (crc != crc32(0, firmware, calc_size)) {
                printk(KERN_ERR "qe-firmware: firmware CRC is invalid\n");
                return -EIO;
        }
 
+       /* Validate ucode lengths and offsets */
+       for (i = 0; i < firmware->count; i++) {
+               const struct qe_microcode *ucode = &firmware->microcode[i];
+               __be32 *code;
+               size_t count;
+
+               if (!ucode->code_offset)
+                       continue;
+
+               code = (void *)firmware + be32_to_cpu(ucode->code_offset);
+               count = be32_to_cpu(ucode->count) * sizeof(*code);
+
+               if (code < firmware || code >= firmware_end ||
+                   code + count < firmware || code + count >= firmware_end) {
+                       printk(KERN_ERR "qe-firmware: invalid ucode offset\n");
+                       return -EIO;
+               }
+       }
+
        /*
         * If the microcode calls for it, split the I-RAM.
         */

I haven't tested this.

--
Kees Cook

Quoted 13 lines · Kees Cook

Hm, looking at this code, I see a few other things that need to be fixed:

1) drivers/tty/serial/ucc_uart.c does not do a be32_to_cpu() conversion on the length test (understandably, a little-endian system has never run this code since it's ppc specific), but it's still wrong:

        if (firmware->header.length != fw->size) {

compare to the firmware loader:

        length = be32_to_cpu(hdr->length);

Quoted 3 lines

2) drivers/soc/fsl/qe/qe.c does not perform bounds checking on the per-microcode offsets, so the uploader might send data outside the firmware buffer. Perhaps:

We do validate the CRC for each microcode, it is unlikely the CRC check can pass if the offset or length is not correct. But you are probably right that it will be safer to check the boundary and fail

quicker before we actually start the CRC check. Will you come up with a formal patch or you want us to deal with it?

Quoted 14 lines · Li Yang

Quoted 13 lines · Kees Cook

Hm, looking at this code, I see a few other things that need to be fixed:

1) drivers/tty/serial/ucc_uart.c does not do a be32_to_cpu() conversion on the length test (understandably, a little-endian system has never run this code since it's ppc specific), but it's still wrong:

        if (firmware->header.length != fw->size) {

compare to the firmware loader:

        length = be32_to_cpu(hdr->length);

Quoted 7 lines

Quoted 3 lines

2) drivers/soc/fsl/qe/qe.c does not perform bounds checking on the per-microcode offsets, so the uploader might send data outside the firmware buffer. Perhaps:

We do validate the CRC for each microcode, it is unlikely the CRC check can pass if the offset or length is not correct. But you are probably right that it will be safer to check the boundary and fail

Right, but a malicious firmware file could still match CRC but trick the kernel code.

Quoted 2 lines

quicker before we actually start the CRC check. Will you come up with a formal patch or you want us to deal with it?

It sounds like Gustavo will be sending one, though I don't think either of us have the hardware to test it with, so if you could do that part, that would be great! :)

The current codebase makes use of one-element arrays in the following form:

struct something {
    int length;
    u8 data[1];
};

struct something *instance;

instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL); instance->length = size;
memcpy(instance->data, source, size);

but the preferred mechanism to declare variable-length types such as these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being inadvertently introduced[3] to the codebase from now on. So, replace the one-element array with a flexible-array member.

Also, make use of the new struct_size() helper to properly calculate the size of struct qe_firmware.

This issue was found with the help of Coccinelle and, audited and fixed _manually_.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/soc/fsl/qe/qe.c | 4 ++--
 include/soc/fsl/qe/qe.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-----Original Message-----
From: Li Yang <[email protected]>
Sent: 2020年5月23日 5:22
To: Kees Cook <[email protected]>
Cc: Gustavo A. R. Silva <[email protected]>; Qiang Zhao <[email protected]>; linuxppc-dev <[email protected]>; moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE <[email protected]>; lkml <[email protected]>; Gustavo A. R. Silva <[email protected]>
Subject: Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper

Quoted 3 lines · Kees Cook

Quoted 1 line · Li Yang

On Mon, May 18, 2020 at 5:57 PM Kees Cook <[email protected]>

Quoted 5 lines · wrote:

Quoted 5 lines

Quoted 5 lines

Hm, looking at this code, I see a few other things that need to be fixed:

1) drivers/tty/serial/ucc_uart.c does not do a be32_to_cpu() conversion on the length test (understandably, a little-endian system has never

run

Quoted 19 lines

Quoted 8 lines

Quoted 8 lines

this code since it's ppc specific), but it's still wrong:

        if (firmware->header.length != fw->size) {

compare to the firmware loader:

        length = be32_to_cpu(hdr->length);

Quoted 7 lines

Quoted 3 lines

2) drivers/soc/fsl/qe/qe.c does not perform bounds checking on the per-microcode offsets, so the uploader might send data outside the firmware buffer. Perhaps:

We do validate the CRC for each microcode, it is unlikely the CRC check can pass if the offset or length is not correct. But you are probably right that it will be safer to check the boundary and fail

Right, but a malicious firmware file could still match CRC but trick the kernel code.

Quoted 6 lines

Quoted 2 lines

quicker before we actually start the CRC check. Will you come up with a formal patch or you want us to deal with it?

It sounds like Gustavo will be sending one, though I don't think either of us have the hardware to test it with, so if you could do that part, that would be great! :)

That will be great. I think Zhao Qiang can help with the testing part.

On Wed, May 23, 2020 at 5:22 PM Li Yang <[email protected]>

Quoted 50 lines

-----Original Message-----
From: Li Yang <[email protected]>
Sent: 2020年5月23日 5:22
To: Kees Cook <[email protected]>
Cc: Gustavo A. R. Silva <[email protected]>; Qiang Zhao <[email protected]>; linuxppc-dev <[email protected]>; moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE <[email protected]>; lkml <[email protected]>; Gustavo A. R. Silva <[email protected]>
Subject: Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper

Quoted 3 lines · Kees Cook

Quoted 1 line · Li Yang

On Mon, May 18, 2020 at 5:57 PM Kees Cook <[email protected]>

Quoted 5 lines · wrote:

Quoted 5 lines

Quoted 5 lines

Hm, looking at this code, I see a few other things that need to be fixed:

1) drivers/tty/serial/ucc_uart.c does not do a be32_to_cpu() conversion on the length test (understandably, a little-endian system has never

run

Quoted 25 lines

Quoted 8 lines

Quoted 8 lines

this code since it's ppc specific), but it's still wrong:

        if (firmware->header.length != fw->size) {

compare to the firmware loader:

        length = be32_to_cpu(hdr->length);

Quoted 7 lines

Quoted 3 lines

2) drivers/soc/fsl/qe/qe.c does not perform bounds checking on the per-microcode offsets, so the uploader might send data outside the firmware buffer. Perhaps:

We do validate the CRC for each microcode, it is unlikely the CRC check can pass if the offset or length is not correct. But you are probably right that it will be safer to check the boundary and fail

Right, but a malicious firmware file could still match CRC but trick the kernel code.

Quoted 2 lines

quicker before we actually start the CRC check. Will you come up with a formal patch or you want us to deal with it?

It sounds like Gustavo will be sending one, though I don't think either of us have the hardware to test it with, so if you could do that part, that would be great! :)

Quoted 2 lines

That will be great. I think Zhao Qiang can help with the testing part.

Now the firmware are loaded in uboot, and kernel will do nothing for it. So testing on it maybe need some extra codes both in driver and dts. In the meanwhile, I am so busy on some high priority work that maybe test work could not be done in time.
Once I am free, I will do it.

Best Regards
Qiang Zhao