[U-Boot] [PATCH v3 0/8] Fix CVE-2018-18440 and CVE-2018-18439

Wed Dec 5 14:13:09 UTC 2018

Hi Simon,

On Wed, 5 Dec 2018 at 06:16, Simon Goldschmidt
<simon.k.r.goldschmidt at gmail.com> wrote:
>
> On Wed, Dec 5, 2018 at 2:13 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Simon,
> >
> > On Tue, 4 Dec 2018 at 04:54, Simon Goldschmidt
> > <simon.k.r.goldschmidt at gmail.com> wrote:
> > >
> > > On Tue, Dec 4, 2018 at 12:45 AM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Mon, 3 Dec 2018 at 12:05, Simon Goldschmidt
> > > > <simon.k.r.goldschmidt at gmail.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > Am Mo., 3. Dez. 2018, 19:20 hat Simon Glass <sjg at chromium.org> geschrieben:
> > > > >>
> > > > >> Hi Simon,
> > > > >>
> > > > >> On Mon, 3 Dec 2018 at 00:50, Simon Goldschmidt
> > > > >> <simon.k.r.goldschmidt at gmail.com> wrote:
> > > > >> >
> > > > >> > Simon,
> > > > >> >
> > > > >> > On Tue, Nov 27, 2018 at 6:45 AM Simon Goldschmidt
> > > > >> > <simon.k.r.goldschmidt at gmail.com> wrote:
> > > > >> > >
> > > > >> > > On Tue, Nov 27, 2018 at 2:02 AM Simon Glass <sjg at chromium.org> wrote:
> > > > >> > > >
> > > > >> > > > Hi Simon,
> > > > >> > > >
> > > > >> > > > On Sat, 17 Nov 2018 at 05:25, Simon Goldschmidt
> > > > >> > > > <simon.k.r.goldschmidt at gmail.com> wrote:
> > > > >> > > > >
> > > > >> > > > > This series fixes CVE-2018-18440 ("insufficient boundary checks in
> > > > >> > > > > filesystem image load") by adding restrictions to the 'load'
> > > > >> > > > > command and fixes CVE-2018-18439 ("insufficient boundary checks in
> > > > >> > > > > network image boot") by adding restrictions to the tftp code.
> > > > >> > > > >
> > > > >> > > > > The functions from lmb.c are used to setup regions of allowed and
> > > > >> > > > > reserved memory. Then, the file size to load is checked against these
> > > > >> > > > > addresses and loading the file is aborted if it would overwrite
> > > > >> > > > > reserved memory.
> > > > >> > > > >
> > > > >> > > > > The memory reservation code is reused from bootm/image.
> > > > >> > > > >
> > > > >> > > > > Changes in v3:
> > > > >> > > > > - No patch changes, but needed to resend since patman added too many cc
> > > > >> > > > >   addresses that gmail seemed to detect as spam :-(
> > > > >> > > > >
> > > > >> > > > > Changes in v2:
> > > > >> > > > > - added code to reserve devicetree reserved-memory in lmb
> > > > >> > > > > - added tftp fixes (patches 7 and 8)
> > > > >> > > > > - fixed a bug in new function lmb_alloc_addr
> > > > >> > > > >
> > > > >> > > > > Simon Goldschmidt (8):
> > > > >> > > > >   lib: lmb: reserving overlapping regions should fail
> > > > >> > > > >   fdt: parse "reserved-memory" for memory reservation
> > > > >> > > > >   lib: lmb: extend lmb for checks at load time
> > > > >> > > > >   fs: prevent overwriting reserved memory
> > > > >> > > > >   bootm: use new common function lmb_init_and_reserve
> > > > >> > > > >   lmb: remove unused extern declaration
> > > > >> > > > >   net: remove CONFIG_MCAST_TFTP
> > > > >> > > > >   tftp: prevent overwriting reserved memory
> > > > >> > > > >
> > > > >> > > > >  README                       |   9 --
> > > > >> > > > >  common/bootm.c               |   8 +-
> > > > >> > > > >  common/image-fdt.c           |  52 ++++++-
> > > > >> > > > >  drivers/net/rtl8139.c        |   9 --
> > > > >> > > > >  drivers/net/tsec.c           |  52 -------
> > > > >> > > > >  drivers/usb/gadget/ether.c   |   3 -
> > > > >> > > > >  fs/fs.c                      |  56 ++++++-
> > > > >> > > > >  include/lmb.h                |   7 +-
> > > > >> > > > >  include/net.h                |  17 ---
> > > > >> > > > >  lib/lmb.c                    |  69 +++++++++
> > > > >> > > > >  net/eth-uclass.c             |   4 -
> > > > >> > > > >  net/eth_legacy.c             |  46 ------
> > > > >> > > > >  net/net.c                    |   9 +-
> > > > >> > > > >  net/tftp.c                   | 289 +++++++----------------------------
> > > > >> > > > >  scripts/config_whitelist.txt |   1 -
> > > > >> > > > >  15 files changed, 232 insertions(+), 399 deletions(-)
> > > > >> > > >
> > > > >> > > > This is great work, but what is missing is a test for lmb.
> > > > >> > >
> > > > >> > > Yeah, well, the tests didn't work on my system and I figured it's
> > > > >> > > better to get the code fixed than to use my time on trying to get the
> > > > >> > > tests running.
> > > > >> > >
> > > > >> > > However, after searching for the required packages and fiddling around
> > > > >> > > some more, I guess I made them work so I could add tests now...
> > > > >> > >
> > > > >> > > I also have work-in-progress for compressing fit image contents (we
> > > > >> > > currently only support uncompressing the kernel). It will switch some
> > > > >> > > 'lmb_reserve' calls to the new 'lmb_alloc_addr' as this is more safe.
> > > > >> > > Maybe I can combine the tests in that series?
> > > > >> >
> > > > >> > After managing to get the tests to run via 'make qcheck' (and 'make
> > > > >> > tests'; had to install much more than listed in 'test/py/README.md'),
> > > > >> > I tried to add tests to 'test/lib/' (next to hexdump.c), but I failed
> > > > >> > to get them run. Even chaning 'test/lib/hexdump.c' to fail did not
> > > > >> > produce errors. Are these tests not included in 'make qcheck'?
> > > > >>
> > > > >> That runs the Python tests which are in test/py/tests. Some of those
> > > > >> tests run compiled-in code (e.g. log_test.c and cmd_ut.c). Is your
> > > > >> test intended to be Python or C?
> > > > >
> > > > >
> > > > > I thought I'd create a unit test under test/lib that calls functions from lmb.c, so that would be C code. Python would not work without adding extra commands to call from Python.
> > > > >
> > > > > There are tests in test/lib, how do I run them?
> > > >
> > > > I suspect it is lib/ since it is holds tests for library functions,
> > > > although hex_to_bin() is an inline function.
> > > >
> > > > Better to put tests in another dir. Maybe test/image ?
> > >
> > > Well, my tests would ensure lib/lmb.c works as expected (especially
> > > for the corner case reported by Frank), so I though test/lib/ would be
> > > good?
> >
> > I suppose so.
> >
> > >
> > > > You can run an individual test with something like:
> > > >
> > > > /tmp/b/sandbox/u-boot  -d /tmp/b/sandbox/arch/sandbox/dts/test.dtb  -c
> > > > "ut dm lib_test_bin2hex"
> > > >
> > > > where /tmp/b/sandbox is the build directory for sandbox.
> > >
> > > OK, that worked, thanks for the hint. Did I miss this in the
> > > documentation somewhere?
> >
> > No, only the help from the 'ut' command. But you could add a patch to
> > test/README perhaps?
>
> Once I have understood what's missing, I could do that ;-)
>
> > > And are these tests executed in a standard test run (e.g. on travis)?
> > > If not, how would they be integrated?
> >
> > So long as they run with 'make check' (or 'make qcheck') then you should be OK.
>
> Since I did not get 'make check' to report a failure when changing
> test/lib/hexdump.c to fail, I don't think these tests are run on 'make
> check'. Where exactly is it defined which tests are run?

See generate_ut_subtest() for the logic.

I actually think this is a bit of a widespread problem. E.g. the
bloblist tests don't run.

Regards,
Simon