From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiko =?ISO-8859-1?Q?St=FCbner?= <heiko@sntech.de>
Date: Tue, 09 Jun 2020 12:45:26 +0200
Subject: [BUG] ut lib lib_rsa_verify_valid crashes on qemu_arm if
 RSA_VERIFY_WITH_PKEY=y
In-Reply-To: <c2c62d38-7867-d225-2e63-695c6405cb95@gmx.de>
References: <d481c877-9a4c-9cea-2abc-5e226c3a31e5@gmx.de>
 <2399481.2eeKMpjr2i@diego> <c2c62d38-7867-d225-2e63-695c6405cb95@gmx.de>
Message-ID: <4540707.Ran6o8JzkR@diego>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Am Dienstag, 9. Juni 2020, 12:22:36 CEST schrieb Heinrich Schuchardt:
> On 09.06.20 12:11, Heiko St?bner wrote:
> > Hi Akashi,
> >
> > Am Dienstag, 9. Juni 2020, 11:22:35 CEST schrieb AKASHI Takahiro:
> >> Heinrich,
> >>
> >> On Tue, Jun 09, 2020 at 03:54:44AM +0000, Heinrich Schuchardt wrote:
> >>> Am June 9, 2020 1:42:14 AM UTC schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> >>>> Heinrich,
> >>>>
> >>>> On Mon, Jun 08, 2020 at 11:08:53PM +0200, Heinrich Schuchardt wrote:
> >>>>> Hello Takahiro,
> >>>>>
> >>>>> when trying to execute command
> >>>>>
> >>>>> ut lib lib_rsa_verify_valid
> >>>>>
> >>>>> on qemu_arm_defconfig with CONFIG_UNIT_TEST=y and
> >>>>> CONFIG_RSA_VERIFY_WITH_PKEY=y it crashes in
> >>>>>
> >>>>> free((void *)prop->modulus) called from
> >>>>> rsa_free_key_prop() called from
> >>>>> rsa_verify_key() called from
> >>>>> rsa_verify_with_pkey().
> >>>>>
> >>>>> Without CONFIG_RSA_VERIFY_WITH_PKEY=y the problem does not occur.
> >>>>> On qemu_arm64_defconfig the problem does not occur.
> >>>>
> >>>> I can't reproduce your problem on v2020.07-rc4 exactly with
> >>>> qemu_arm64_defconfig + PKEY=y:
> >>>>
> >>>> U-Boot 2020.07-rc4-dirty (Jun 09 2020 - 10:33:30 +0900)
> >>>>
> >>>> ...
> >>>>
> >>>> => ut lib
> >>>> Running 11 lib tests
> >>>> Test: lib_asn1_pkcs7
> >>>> Test: lib_asn1_pkey
> >>>> Test: lib_asn1_x509
> >>>> Test: lib_memcpy
> >>>> Test: lib_memmove
> >>>> Test: lib_memset
> >>>> Test: lib_rsa_verify_invalid
> >>>> Test: lib_rsa_verify_valid
> >>>> Test: lib_test_bin2hex
> >>>> Test: lib_test_hex2bin
> >>>> Test: lib_test_hex_to_bin
> >>>> Failures: 0
> >>>>
> >>>>
> >>>> -Takahiro Akashi
> >>>
> >>> As said I only see the problem with 32 bit qemu_arm_defconfig.
> >>
> >> Okay. I think that the size of rrtmp variable is not good enough
> >> and when it is handed over to br_i32_decode(), it accidentally
> >> destroys (*prop)->modulus.
> >>
> >> Heiko's patch:
> >>   https://lists.denx.de/pipermail/u-boot/2020-May/413101.html
> >> will also fix this issue, but I'm not yet confident that
> >> the solution here, doubling max_rsa_size, is a right approach.
> >
> > The algorithm does write outside its memory area with a big enough key
> > and I have to confess I had a hard time understanding it ;-) .
> >
> > So what would be needed to get more confidence?
> >
> > Because for me it looks like either the algorithm does strange things
> > or the memory area is too small - I don't really see a third option ;-) .
> 
> Your patch allocates unnecessary memory for key sizes below 4096 and
> allocates too little for key sizes above 4096 bits. So with a bad key
> size you can crash the code again.

valid point :-) .

I guess I got sidetracked with u-boot signature handling only doing up
to 4096bit right now and was not questioning the existing usage of
max_rsa_size enough.