On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:

On Mon, May 18, 2020 at 5:57 PM Kees Cook [off-list ref] wrote:

quoted

On Mon, May 18, 2020 at 05:19:04PM -0500, Gustavo A. R. Silva wrote:

quoted

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 <gustavoars@kernel.org>
---
 drivers/soc/fsl/qe/qe.c | 4 ++--
 include/soc/fsl/qe/qe.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 447146861c2c1..2df20d6f85fa4 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c

@@ -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++)
              /*

diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
index e282ac01ec081..3feddfec9f87d 100644
--- a/include/soc/fsl/qe/qe.h
+++ b/include/soc/fsl/qe/qe.h

@@ -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

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:

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?

Li,

I will send a proper patch for this.

Thanks
--
Gustavo

quoted

diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 447146861c2c..c4e0bc452f03 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c

@@ -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

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