* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-20 23:52 ` Li Yang
0 siblings, 0 replies; 30+ messages in thread
From: Li Yang @ 2020-05-20 23:52 UTC (permalink / raw)
To: Kees Cook
Cc: Gustavo A. R. Silva, lkml, Gustavo A. R. Silva, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Qiang Zhao
On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, May 18, 2020 at 05:19:04PM -0500, Gustavo A. R. Silva wrote:
> > 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?
>
>
> 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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-20 23:52 ` Li Yang
0 siblings, 0 replies; 30+ messages in thread
From: Li Yang @ 2020-05-20 23:52 UTC (permalink / raw)
To: Kees Cook
Cc: Gustavo A. R. Silva, lkml, Gustavo A. R. Silva, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Qiang Zhao
On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, May 18, 2020 at 05:19:04PM -0500, Gustavo A. R. Silva wrote:
> > 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?
>
>
> 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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
2020-05-20 23:52 ` Li Yang
(?)
@ 2020-05-21 0:01 ` Gustavo A. R. Silva
-1 siblings, 0 replies; 30+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-21 0:01 UTC (permalink / raw)
To: Li Yang
Cc: Kees Cook, Qiang Zhao, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, lkml,
Gustavo A. R. Silva
On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, May 18, 2020 at 05:19:04PM -0500, Gustavo A. R. Silva wrote:
> > > 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
> >
> >
> > 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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-21 0:01 ` Gustavo A. R. Silva
0 siblings, 0 replies; 30+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-21 0:01 UTC (permalink / raw)
To: Li Yang
Cc: Kees Cook, Gustavo A. R. Silva, lkml, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Qiang Zhao
On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, May 18, 2020 at 05:19:04PM -0500, Gustavo A. R. Silva wrote:
> > > 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
> >
> >
> > 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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-21 0:01 ` Gustavo A. R. Silva
0 siblings, 0 replies; 30+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-21 0:01 UTC (permalink / raw)
To: Li Yang
Cc: Kees Cook, Gustavo A. R. Silva, lkml, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Qiang Zhao
On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, May 18, 2020 at 05:19:04PM -0500, Gustavo A. R. Silva wrote:
> > > 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
> >
> >
> > 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
2020-05-20 23:52 ` Li Yang
(?)
@ 2020-05-21 3:24 ` Kees Cook
-1 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2020-05-21 3:24 UTC (permalink / raw)
To: Li Yang
Cc: Gustavo A. R. Silva, Qiang Zhao, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, lkml,
Gustavo A. R. Silva
On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
> > 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
Right, but a malicious firmware file could still match CRC but trick the
kernel code.
> 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! :)
--
Kees Cook
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-21 3:24 ` Kees Cook
0 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2020-05-21 3:24 UTC (permalink / raw)
To: Li Yang
Cc: Gustavo A. R. Silva, lkml, Gustavo A. R. Silva, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Qiang Zhao
On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
> > 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
Right, but a malicious firmware file could still match CRC but trick the
kernel code.
> 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! :)
--
Kees Cook
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-21 3:24 ` Kees Cook
0 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2020-05-21 3:24 UTC (permalink / raw)
To: Li Yang
Cc: Gustavo A. R. Silva, lkml, Gustavo A. R. Silva, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Qiang Zhao
On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
> > 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
Right, but a malicious firmware file could still match CRC but trick the
kernel code.
> 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! :)
--
Kees Cook
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
2020-05-21 3:24 ` Kees Cook
(?)
@ 2020-05-22 21:21 ` Li Yang
-1 siblings, 0 replies; 30+ messages in thread
From: Li Yang @ 2020-05-22 21:21 UTC (permalink / raw)
To: Kees Cook
Cc: Gustavo A. R. Silva, Qiang Zhao, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, lkml,
Gustavo A. R. Silva
On Wed, May 20, 2020 at 10:24 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> > On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
> > > 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
>
> Right, but a malicious firmware file could still match CRC but trick the
> kernel code.
>
> > 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.
Regards,
Leo
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-22 21:21 ` Li Yang
0 siblings, 0 replies; 30+ messages in thread
From: Li Yang @ 2020-05-22 21:21 UTC (permalink / raw)
To: Kees Cook
Cc: Gustavo A. R. Silva, lkml, Gustavo A. R. Silva, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Qiang Zhao
On Wed, May 20, 2020 at 10:24 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> > On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
> > > 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
>
> Right, but a malicious firmware file could still match CRC but trick the
> kernel code.
>
> > 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.
Regards,
Leo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-22 21:21 ` Li Yang
0 siblings, 0 replies; 30+ messages in thread
From: Li Yang @ 2020-05-22 21:21 UTC (permalink / raw)
To: Kees Cook
Cc: Gustavo A. R. Silva, lkml, Gustavo A. R. Silva, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Qiang Zhao
On Wed, May 20, 2020 at 10:24 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> > On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
> > > 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
>
> Right, but a malicious firmware file could still match CRC but trick the
> kernel code.
>
> > 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.
Regards,
Leo
^ permalink raw reply [flat|nested] 30+ messages in thread* RE: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
2020-05-22 21:21 ` Li Yang
(?)
@ 2020-05-25 2:47 ` Qiang Zhao
-1 siblings, 0 replies; 30+ messages in thread
From: Qiang Zhao @ 2020-05-25 2:47 UTC (permalink / raw)
To: Leo Li, Leo Li, Kees Cook
Cc: Gustavo A. R. Silva, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, lkml,
Gustavo A. R. Silva
On Wed, May 23, 2020 at 5:22 PM Li Yang <leoyang.li@nxp.com>
> -----Original Message-----
> From: Li Yang <leoyang.li@nxp.com>
> Sent: 2020年5月23日 5:22
> To: Kees Cook <keescook@chromium.org>
> Cc: Gustavo A. R. Silva <gustavoars@kernel.org>; Qiang Zhao
> <qiang.zhao@nxp.com>; linuxppc-dev <linuxppc-dev@lists.ozlabs.org>;
> moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>; lkml <linux-kernel@vger.kernel.org>;
> Gustavo A. R. Silva <gustavo@embeddedor.com>
> Subject: Re: [PATCH] soc: fsl: qe: Replace one-element array and use
> struct_size() helper
>
> On Wed, May 20, 2020 at 10:24 PM Kees Cook <keescook@chromium.org>
> wrote:
> >
> > On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> > > On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org>
> wrote:
> > > > 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
> >
> > Right, but a malicious firmware file could still match CRC but trick
> > the kernel code.
> >
> > > 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.
>
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
^ permalink raw reply [flat|nested] 30+ messages in thread* RE: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-25 2:47 ` Qiang Zhao
0 siblings, 0 replies; 30+ messages in thread
From: Qiang Zhao @ 2020-05-25 2:47 UTC (permalink / raw)
To: Leo Li, Leo Li, Kees Cook
Cc: Gustavo A. R. Silva, linuxppc-dev, Gustavo A. R. Silva,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, lkml
On Wed, May 23, 2020 at 5:22 PM Li Yang <leoyang.li@nxp.com>
> -----Original Message-----
> From: Li Yang <leoyang.li@nxp.com>
> Sent: 2020年5月23日 5:22
> To: Kees Cook <keescook@chromium.org>
> Cc: Gustavo A. R. Silva <gustavoars@kernel.org>; Qiang Zhao
> <qiang.zhao@nxp.com>; linuxppc-dev <linuxppc-dev@lists.ozlabs.org>;
> moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>; lkml <linux-kernel@vger.kernel.org>;
> Gustavo A. R. Silva <gustavo@embeddedor.com>
> Subject: Re: [PATCH] soc: fsl: qe: Replace one-element array and use
> struct_size() helper
>
> On Wed, May 20, 2020 at 10:24 PM Kees Cook <keescook@chromium.org>
> wrote:
> >
> > On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> > > On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org>
> wrote:
> > > > 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
> >
> > Right, but a malicious firmware file could still match CRC but trick
> > the kernel code.
> >
> > > 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.
>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread* RE: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-25 2:47 ` Qiang Zhao
0 siblings, 0 replies; 30+ messages in thread
From: Qiang Zhao @ 2020-05-25 2:47 UTC (permalink / raw)
To: Leo Li, Leo Li, Kees Cook
Cc: Gustavo A. R. Silva, linuxppc-dev, Gustavo A. R. Silva,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, lkml
On Wed, May 23, 2020 at 5:22 PM Li Yang <leoyang.li@nxp.com>
> -----Original Message-----
> From: Li Yang <leoyang.li@nxp.com>
> Sent: 2020年5月23日 5:22
> To: Kees Cook <keescook@chromium.org>
> Cc: Gustavo A. R. Silva <gustavoars@kernel.org>; Qiang Zhao
> <qiang.zhao@nxp.com>; linuxppc-dev <linuxppc-dev@lists.ozlabs.org>;
> moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>; lkml <linux-kernel@vger.kernel.org>;
> Gustavo A. R. Silva <gustavo@embeddedor.com>
> Subject: Re: [PATCH] soc: fsl: qe: Replace one-element array and use
> struct_size() helper
>
> On Wed, May 20, 2020 at 10:24 PM Kees Cook <keescook@chromium.org>
> wrote:
> >
> > On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> > > On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org>
> wrote:
> > > > 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
> >
> > Right, but a malicious firmware file could still match CRC but trick
> > the kernel code.
> >
> > > 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.
>
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
2020-05-25 2:47 ` Qiang Zhao
(?)
@ 2020-05-26 19:56 ` Li Yang
-1 siblings, 0 replies; 30+ messages in thread
From: Li Yang @ 2020-05-26 19:56 UTC (permalink / raw)
To: Qiang Zhao
Cc: Kees Cook, Gustavo A. R. Silva, linuxppc-dev,
Gustavo A. R. Silva,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, lkml
On Sun, May 24, 2020 at 9:49 PM Qiang Zhao <qiang.zhao@nxp.com> wrote:
>
> On Wed, May 23, 2020 at 5:22 PM Li Yang <leoyang.li@nxp.com>
> > -----Original Message-----
> > From: Li Yang <leoyang.li@nxp.com>
> > Sent: 2020年5月23日 5:22
> > To: Kees Cook <keescook@chromium.org>
> > Cc: Gustavo A. R. Silva <gustavoars@kernel.org>; Qiang Zhao
> > <qiang.zhao@nxp.com>; linuxppc-dev <linuxppc-dev@lists.ozlabs.org>;
> > moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> > <linux-arm-kernel@lists.infradead.org>; lkml <linux-kernel@vger.kernel.org>;
> > Gustavo A. R. Silva <gustavo@embeddedor.com>
> > Subject: Re: [PATCH] soc: fsl: qe: Replace one-element array and use
> > struct_size() helper
> >
> > On Wed, May 20, 2020 at 10:24 PM Kees Cook <keescook@chromium.org>
> > wrote:
> > >
> > > On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> > > > On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org>
> > wrote:
> > > > > 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
> > >
> > > Right, but a malicious firmware file could still match CRC but trick
> > > the kernel code.
> > >
> > > > 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.
> >
>
> 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.
Thanks. You are right that most of the QE drivers doesn't support
requesting firmware in kernel except the ucc_uart. So it probably can
be tested with that driver without requiring code change.
>
> Best Regards
> Qiang Zhao
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-26 19:56 ` Li Yang
0 siblings, 0 replies; 30+ messages in thread
From: Li Yang @ 2020-05-26 19:56 UTC (permalink / raw)
To: Qiang Zhao
Cc: Kees Cook, Gustavo A. R. Silva, lkml, Gustavo A. R. Silva,
linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
On Sun, May 24, 2020 at 9:49 PM Qiang Zhao <qiang.zhao@nxp.com> wrote:
>
> On Wed, May 23, 2020 at 5:22 PM Li Yang <leoyang.li@nxp.com>
> > -----Original Message-----
> > From: Li Yang <leoyang.li@nxp.com>
> > Sent: 2020年5月23日 5:22
> > To: Kees Cook <keescook@chromium.org>
> > Cc: Gustavo A. R. Silva <gustavoars@kernel.org>; Qiang Zhao
> > <qiang.zhao@nxp.com>; linuxppc-dev <linuxppc-dev@lists.ozlabs.org>;
> > moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> > <linux-arm-kernel@lists.infradead.org>; lkml <linux-kernel@vger.kernel.org>;
> > Gustavo A. R. Silva <gustavo@embeddedor.com>
> > Subject: Re: [PATCH] soc: fsl: qe: Replace one-element array and use
> > struct_size() helper
> >
> > On Wed, May 20, 2020 at 10:24 PM Kees Cook <keescook@chromium.org>
> > wrote:
> > >
> > > On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> > > > On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org>
> > wrote:
> > > > > 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
> > >
> > > Right, but a malicious firmware file could still match CRC but trick
> > > the kernel code.
> > >
> > > > 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.
> >
>
> 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.
Thanks. You are right that most of the QE drivers doesn't support
requesting firmware in kernel except the ucc_uart. So it probably can
be tested with that driver without requiring code change.
>
> Best Regards
> Qiang Zhao
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
@ 2020-05-26 19:56 ` Li Yang
0 siblings, 0 replies; 30+ messages in thread
From: Li Yang @ 2020-05-26 19:56 UTC (permalink / raw)
To: Qiang Zhao
Cc: Kees Cook, Gustavo A. R. Silva, lkml, Gustavo A. R. Silva,
linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
On Sun, May 24, 2020 at 9:49 PM Qiang Zhao <qiang.zhao@nxp.com> wrote:
>
> On Wed, May 23, 2020 at 5:22 PM Li Yang <leoyang.li@nxp.com>
> > -----Original Message-----
> > From: Li Yang <leoyang.li@nxp.com>
> > Sent: 2020年5月23日 5:22
> > To: Kees Cook <keescook@chromium.org>
> > Cc: Gustavo A. R. Silva <gustavoars@kernel.org>; Qiang Zhao
> > <qiang.zhao@nxp.com>; linuxppc-dev <linuxppc-dev@lists.ozlabs.org>;
> > moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> > <linux-arm-kernel@lists.infradead.org>; lkml <linux-kernel@vger.kernel.org>;
> > Gustavo A. R. Silva <gustavo@embeddedor.com>
> > Subject: Re: [PATCH] soc: fsl: qe: Replace one-element array and use
> > struct_size() helper
> >
> > On Wed, May 20, 2020 at 10:24 PM Kees Cook <keescook@chromium.org>
> > wrote:
> > >
> > > On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> > > > On Mon, May 18, 2020 at 5:57 PM Kees Cook <keescook@chromium.org>
> > wrote:
> > > > > 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
> > >
> > > Right, but a malicious firmware file could still match CRC but trick
> > > the kernel code.
> > >
> > > > 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.
> >
>
> 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.
Thanks. You are right that most of the QE drivers doesn't support
requesting firmware in kernel except the ucc_uart. So it probably can
be tested with that driver without requiring code change.
>
> Best Regards
> Qiang Zhao
^ permalink raw reply [flat|nested] 30+ messages in thread