| Message ID | 20200518221904.GA22274@embeddedor (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 661ea25e5319d0ceaeba80dbc2e083245d91f57a |
| Headers | show |
| Series | soc: fsl: qe: Replace one-element array and use struct_size() helper | expand |
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: 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.
On 2020/5/19 6:19, Gustavo A. R. Silva <gustavoars@kernel.org> wrote: > -----Original Message----- > From: Gustavo A. R. Silva <gustavoars@kernel.org> > Sent: 2020年5月19日 6:19 > To: Qiang Zhao <qiang.zhao@nxp.com>; Leo Li <leoyang.li@nxp.com> > Cc: linuxppc-dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; Gustavo A. R. Silva <gustavo@embeddedor.com>; > Kees Cook <keescook@chromium.org> > 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&data=02%7C01%7Cqiang. > zhao%40nxp.com%7Cb058bcb9af134df8446808d7fb78cfa9%7C686ea1d3bc2b > 4c6fa92cd99c5c301635%7C0%7C0%7C637254368610203908&sdata=70 > a6VBg3oWQf9a5KICuCEuIj6gw57NKYhNv2JL8JdDY%3D&reserved=0 > [2] > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c > om%2FKSPP%2Flinux%2Fissues%2F21&data=02%7C01%7Cqiang.zhao%40 > nxp.com%7Cb058bcb9af134df8446808d7fb78cfa9%7C686ea1d3bc2b4c6fa92c > d99c5c301635%7C0%7C0%7C637254368610213901&sdata=kVyNBYHbm > a5jCO9pbkoHMetHEYGvWDq6Xw%2BBeC7uHII%3D&reserved=0 > [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 [>] Reviewed-by: Qiang Zhao <qiang.zhao@nxp.com> Best Regards 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
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
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! :)
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
On Mon, May 18, 2020 at 5:16 PM Gustavo A. R. Silva <gustavoars@kernel.org> 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(-) Applied for next. Thanks. Regards, Leo
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
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
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));
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(-)