From: Li Yang <leoyang.li@nxp.com>
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
Date: Fri, 22 May 2020 16:21:38 -0500 [thread overview]
Message-ID: <CADRPPNTuUUVOHs76JVzELcsyRH_LSi2PGML1t2wob+45LJCXvA@mail.gmail.com> (raw)
In-Reply-To: <202005202022.588918E61@keescook>
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
WARNING: multiple messages have this Message-ID (diff)
From: Li Yang <leoyang.li@nxp.com>
To: Kees Cook <keescook@chromium.org>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
lkml <linux-kernel@vger.kernel.org>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
<linux-arm-kernel@lists.infradead.org>,
Qiang Zhao <qiang.zhao@nxp.com>
Subject: Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
Date: Fri, 22 May 2020 16:21:38 -0500 [thread overview]
Message-ID: <CADRPPNTuUUVOHs76JVzELcsyRH_LSi2PGML1t2wob+45LJCXvA@mail.gmail.com> (raw)
In-Reply-To: <202005202022.588918E61@keescook>
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
WARNING: multiple messages have this Message-ID (diff)
From: Li Yang <leoyang.li@nxp.com>
To: Kees Cook <keescook@chromium.org>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
lkml <linux-kernel@vger.kernel.org>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
<linux-arm-kernel@lists.infradead.org>,
Qiang Zhao <qiang.zhao@nxp.com>
Subject: Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
Date: Fri, 22 May 2020 16:21:38 -0500 [thread overview]
Message-ID: <CADRPPNTuUUVOHs76JVzELcsyRH_LSi2PGML1t2wob+45LJCXvA@mail.gmail.com> (raw)
In-Reply-To: <202005202022.588918E61@keescook>
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
next prev parent reply other threads:[~2020-05-22 21:22 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-18 22:19 [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper Gustavo A. R. Silva
2020-05-18 22:19 ` Gustavo A. R. Silva
2020-05-18 22:19 ` Gustavo A. R. Silva
2020-05-18 22:56 ` Kees Cook
2020-05-18 22:56 ` Kees Cook
2020-05-18 22:56 ` Kees Cook
2020-05-20 23:52 ` Li Yang
2020-05-20 23:52 ` Li Yang
2020-05-20 23:52 ` Li Yang
2020-05-21 0:01 ` Gustavo A. R. Silva
2020-05-21 0:01 ` Gustavo A. R. Silva
2020-05-21 0:01 ` Gustavo A. R. Silva
2020-05-21 3:24 ` Kees Cook
2020-05-21 3:24 ` Kees Cook
2020-05-21 3:24 ` Kees Cook
2020-05-22 21:21 ` Li Yang [this message]
2020-05-22 21:21 ` Li Yang
2020-05-22 21:21 ` Li Yang
2020-05-25 2:47 ` Qiang Zhao
2020-05-25 2:47 ` Qiang Zhao
2020-05-25 2:47 ` Qiang Zhao
2020-05-26 19:56 ` Li Yang
2020-05-26 19:56 ` Li Yang
2020-05-26 19:56 ` Li Yang
2020-05-19 3:37 ` Qiang Zhao
2020-05-19 3:37 ` Qiang Zhao
2020-05-19 3:37 ` Qiang Zhao
2020-05-22 21:23 ` Li Yang
2020-05-22 21:23 ` Li Yang
2020-05-22 21:23 ` Li Yang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADRPPNTuUUVOHs76JVzELcsyRH_LSi2PGML1t2wob+45LJCXvA@mail.gmail.com \
--to=leoyang.li@nxp.com \
--cc=gustavo@embeddedor.com \
--cc=gustavoars@kernel.org \
--cc=keescook@chromium.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=qiang.zhao@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.