cbc mode broken in rk3288 driver

cbc mode broken in rk3288 driver

[Ard Biesheuvel]

Hello all,

While playing around with the fuzz tests on kernelci.org (which has a
couple of rk3288 based boards for boot testing), I noticed that the
rk3288 cbc mode driver is still broken (both AES and DES fail).

For instance, one of the runs failed with

 alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on
test vector \"random: len=6848 klen=32\", cfg=\"random: may_sleep
use_digest src_divs=[93.41%@+1655, 2.19%@+3968, 4.40%@+22]\"

(but see below for the details of a few runs)

However, more importantly, it looks like the driver violates the
scatterlist API, by assuming that sg entries are always mapped and
that sg_virt() and/or page_address(sg_page()) can always be called on
arbitrary scatterlist entries

The failures in question all occur with inputs whose size > PAGE_SIZE,
so it looks like the PAGE_SIZE limit is interacting poorly with the
way the next IV is obtained.

Broken CBC is a recipe for disaster, and so this should really be
fixed, or the driver disabled.

-- 
Ard.


https://storage.kernelci.org/ardb/for-kernelci/v5.3-rc1-195-gd84aa2e87b0e/arm/multi_v7_defconfig/gcc-8/lab-collabora/boot-rk3288-veyron-jaq.html
https://storage.kernelci.org/ardb/for-kernelci/v5.3-rc1-195-gd84aa2e87b0e/arm/multi_v7_defconfig+CONFIG_EFI=y+CONFIG_ARM_LPAE=y/gcc-8/lab-collabora/boot-rk3288-veyron-jaq.html
https://storage.kernelci.org/ardb/for-kernelci/v5.3-rc1-195-gd84aa2e87b0e/arm/multi_v7_defconfig+CONFIG_SMP=n/gcc-8/lab-collabora/boot-rk3288-veyron-jaq.html

Re: cbc mode broken in rk3288 driver

[Elon Zhang]

Hi Ard,

I will try to fix this bug. Furthermore, I will submit a patch to  set 
crypto node default disable in rk3288.dtsi.

On 8/20/2019 23:45, Ard Biesheuvel wrote:
> Hello all,
>
> While playing around with the fuzz tests on kernelci.org (which has a
> couple of rk3288 based boards for boot testing), I noticed that the
> rk3288 cbc mode driver is still broken (both AES and DES fail).
>
> For instance, one of the runs failed with
>
>   alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on
> test vector \"random: len=6848 klen=32\", cfg=\"random: may_sleep
> use_digest src_divs=[93.41%@+1655, 2.19%@+3968, 4.40%@+22]\"
>
> (but see below for the details of a few runs)
>
> However, more importantly, it looks like the driver violates the
> scatterlist API, by assuming that sg entries are always mapped and
> that sg_virt() and/or page_address(sg_page()) can always be called on
> arbitrary scatterlist entries
>
> The failures in question all occur with inputs whose size > PAGE_SIZE,
> so it looks like the PAGE_SIZE limit is interacting poorly with the
> way the next IV is obtained.
>
> Broken CBC is a recipe for disaster, and so this should really be
> fixed, or the driver disabled.
>

Re: cbc mode broken in rk3288 driver

[Ard Biesheuvel]

On Fri, 23 Aug 2019 at 10:10, Elon Zhang <zhangzj@rock-chips.com> wrote:
>
> Hi Ard,
>
> I will try to fix this bug.

Good

> Furthermore, I will submit a patch to  set
> crypto node default disable in rk3288.dtsi.
>

Please don't. The ecb mode works fine, and 'fixing' the DT only helps
if you use the one that ships with the kernel, which is not always the
case.



> On 8/20/2019 23:45, Ard Biesheuvel wrote:
> > Hello all,
> >
> > While playing around with the fuzz tests on kernelci.org (which has a
> > couple of rk3288 based boards for boot testing), I noticed that the
> > rk3288 cbc mode driver is still broken (both AES and DES fail).
> >
> > For instance, one of the runs failed with
> >
> >   alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on
> > test vector \"random: len=6848 klen=32\", cfg=\"random: may_sleep
> > use_digest src_divs=[93.41%@+1655, 2.19%@+3968, 4.40%@+22]\"
> >
> > (but see below for the details of a few runs)
> >
> > However, more importantly, it looks like the driver violates the
> > scatterlist API, by assuming that sg entries are always mapped and
> > that sg_virt() and/or page_address(sg_page()) can always be called on
> > arbitrary scatterlist entries
> >
> > The failures in question all occur with inputs whose size > PAGE_SIZE,
> > so it looks like the PAGE_SIZE limit is interacting poorly with the
> > way the next IV is obtained.
> >
> > Broken CBC is a recipe for disaster, and so this should really be
> > fixed, or the driver disabled.
> >
>
>

Re: cbc mode broken in rk3288 driver

[Elon Zhang]

On 8/23/2019 15:33, Ard Biesheuvel wrote:
> On Fri, 23 Aug 2019 at 10:10, Elon Zhang <zhangzj@rock-chips.com> wrote:
>> Hi Ard,
>>
>> I will try to fix this bug.
> Good
>
>> Furthermore, I will submit a patch to  set
>> crypto node default disable in rk3288.dtsi.
>>
> Please don't. The ecb mode works fine, and 'fixing' the DT only helps
> if you use the one that ships with the kernel, which is not always the
> case.
>
But crypto node default 'okay' in SoC dtsi is not good since not all 
boards need this

hardware function. It is better that default 'disbale' in SoC dtsi and 
enabled in specific

board dts.

>
>> On 8/20/2019 23:45, Ard Biesheuvel wrote:
>>> Hello all,
>>>
>>> While playing around with the fuzz tests on kernelci.org (which has a
>>> couple of rk3288 based boards for boot testing), I noticed that the
>>> rk3288 cbc mode driver is still broken (both AES and DES fail).
>>>
>>> For instance, one of the runs failed with
>>>
>>>    alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on
>>> test vector \"random: len=6848 klen=32\", cfg=\"random: may_sleep
>>> use_digest src_divs=[93.41%@+1655, 2.19%@+3968, 4.40%@+22]\"
>>>
>>> (but see below for the details of a few runs)
>>>
>>> However, more importantly, it looks like the driver violates the
>>> scatterlist API, by assuming that sg entries are always mapped and
>>> that sg_virt() and/or page_address(sg_page()) can always be called on
>>> arbitrary scatterlist entries
>>>
>>> The failures in question all occur with inputs whose size > PAGE_SIZE,
>>> so it looks like the PAGE_SIZE limit is interacting poorly with the
>>> way the next IV is obtained.
>>>
>>> Broken CBC is a recipe for disaster, and so this should really be
>>> fixed, or the driver disabled.
>>>
>>
>
>

Re: cbc mode broken in rk3288 driver

[Ard Biesheuvel]

On Fri, 23 Aug 2019 at 11:21, Elon Zhang <zhangzj@rock-chips.com> wrote:
>
>
> On 8/23/2019 15:33, Ard Biesheuvel wrote:
> > On Fri, 23 Aug 2019 at 10:10, Elon Zhang <zhangzj@rock-chips.com> wrote:
> >> Hi Ard,
> >>
> >> I will try to fix this bug.
> > Good
> >
> >> Furthermore, I will submit a patch to  set
> >> crypto node default disable in rk3288.dtsi.
> >>
> > Please don't. The ecb mode works fine, and 'fixing' the DT only helps
> > if you use the one that ships with the kernel, which is not always the
> > case.
> >
> But crypto node default 'okay' in SoC dtsi is not good since not all
> boards need this
>
> hardware function. It is better that default 'disbale' in SoC dtsi and
> enabled in specific
>
> board dts.
>

It is not a property of the board whether the crypto accelerator is
needed or not, it is a property of the use case in which the board is
being put to use.

Pretending a h/w block does not exist just because the Linux driver is
broken is not the way to fix this. Disable the driver as long as it is
broken, and re-enable it once it is fixed. And please don't touch the
dts - it describes the hardware, not the software.