RE: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper

[Qiang Zhao]

On Wed, May 23, 2020 at 5:22 PM Li Yang <leoyang.li@xxxxxxx>
> -----Original Message-----
> From: Li Yang <leoyang.li@xxxxxxx>
> Sent: 2020å5æ23æ 5:22
> To: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>; Qiang Zhao
> <qiang.zhao@xxxxxxx>; linuxppc-dev <linuxppc-dev@xxxxxxxxxxxxxxxx>;
> moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; lkml <linux-kernel@xxxxxxxxxxxxxxx>;
> Gustavo A. R. Silva <gustavo@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
> 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