Date:   Thu, 24 Feb 2022 09:07:47 +0200
From:   Gilad Ben-Yossef <gilad@...yossef.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Ofir Drang <ofir.drang@....com>,
        Corentin Labbe <clabbe.montjoie@...il.com>,
        stable <stable@...r.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] crypto: drbg: fix crypto api abuse

Hi Eric,

On Thu, Feb 24, 2022 at 3:53 AM Eric Biggers <ebiggers@...nel.org> wrote:
>
> On Wed, Feb 23, 2022 at 05:47:25PM -0800, Eric Biggers wrote:
> > On Wed, Feb 23, 2022 at 10:04:00AM +0200, Gilad Ben-Yossef wrote:
> > > the drbg code was binding the same buffer to two different
> > > scatter gather lists and submitting those as source and
> > > destination to a crypto api operation, thus potentially
> > > causing HW crypto drivers to perform overlapping DMA
> > > mappings which are not aware it is the same buffer.
> > >
> > > This can have serious consequences of data corruption of
> > > internal DRBG buffers and wrong RNG output.
> > >
> > > Fix this by reusing the same scatter gatther list for both
> > > src and dst.
> > >
> > > Signed-off-by: Gilad Ben-Yossef <gilad@...yossef.com>
> > > Reported-by: Corentin Labbe <clabbe.montjoie@...il.com>
> > > Tested-by: Corentin Labbe <clabbe.montjoie@...il.com>
> > > Tested-on: r8a7795-salvator-x
> > > Tested-on: xilinx-zc706
> > > Fixes: 43490e8046b5d ("crypto: drbg - in-place cipher operation for CTR")
> > > Cc: stable@...r.kernel.org
> >
> > Where is it documented and tested that the API doesn't allow this?
> > I wasn't aware of this case; it sounds perfectly allowed to me.
> > There might be a lot of other users who do this, not just drbg.c.
> >
>
> Just quickly looking through the code I maintain, there is another place that
> uses scatterlists like this: in fscrypt_crypt_block() in fs/crypto/crypto.c, the
> source and destination can be the same.  That's just the code I maintain; I'm
> sure if you looked through the whole kernel you'd find a lot more.
>
> This sounds more like a driver bug, and a case we need to add self-tests for.

Thank you for the feedback. That is a very good question. Indeed, I
agree with you that in an ideal world the internal implementation details of DMA
mapping would not pop up and interfere with higher level layer logic.

Let me describe my point of view and I would be very happy to hear
where I am wrong:

The root cause underlying this is that, of course,  hardware crypto
drivers map the sglists passed to them for DMA . Indeed, we require
input to crypto
API as sglists of DMAable buffers (and not, say stack allocated buffers) because
of this. So far I am just stating the obvious...

Now, it looks like the DMA api, accessed via dma_map_sg(), does not
like overlapping mappings. The bug report that triggered this patch (see:
https://lkml.org/lkml/2022/2/20/240) was an oops message including this
warning: "DMA-API: ccree e6601000.crypto: cacheline tracking EEXIST,
overlapping mappings aren't supported".

The messages comes from add_dma_entry() in kernel.dma/debug.c,
because, as stated in the commit message that added this check in May 2021:

"Since, overlapping mappings are not supported by the DMA API we
should report an error if active_cacheline_insert returns -EEXIST."
(https://lkml.org/lkml/2021/5/18/572)

For now, I will take it at a given that this is proper and you do not
consider this
an issue in the DMA API.

Now, driver writers are of course aware of this DMA API limitation and thus we
check if the src sglist is the same as the dst sglist and if so only map once.
However, the underlying assumption is that the buffers pointed by different
sglists do not overlap. We do not iterate over all the sglist trying
to find overlaps.

When I see "we", it is because this behavior is not unique to the ccree driver:

Here is the same logic from a marvell cesa driver:
https://elixir.bootlin.com/linux/latest/source/drivers/crypto/marvell/cesa/cipher.c#L326

Here it is again in the camm driver:
https://elixir.bootlin.com/linux/latest/source/drivers/crypto/caam/caamalg.c#L1619

I do believe that at least all crypto HW drivers apply the same logic.

Of course, we can ask that every HW crypto driver (and possibly any other
sglist using HW driver) will add logic that scans each sglist for
overlapping buffers
and if found use a more sophisticated mapping (easy for a simple
sglist that has one buffer
identical to some other sglist, maybe more complicated if the overlap
is not identity).
The storage drivers sort of already do on some level, although I think
on a higher abstraction
layer than the drivers themselves if I'm not mistaken, though for
performance reasons.
This is certainly DOABLE in the sense that it can be achieved.

However, I don't think this is desirable. This will add non trivial
code with non trivial runtime
costs just to spot these cases. And we will need to fix ALL the hw
drivers, because, to the best
of my knowledge, none of them do this right now.

The remaining option is to enforce the rule of no overlap between
different sglists passed to the
crypto API. This seems much easier to me. Indeed, the fix I sent is a
one liner. I suspect all
other fixes are similar and I assume (but did not check) that there
are not many of those.
Indeed, I think it is much easier to impose the required limitation at
the API caller level.
It is not pretty, nor "just", but easier, I think.

I hope I've managed to explain my logic here.

I will note that even if we decide to follow the other route, we do
need to document and fix
probably every hw crypto (and possibly others) driver out there,
because AFAIK, no one is taking
into account this possibility right now.

Cheers,
Gilad


--
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

Powered by blists - more mailing lists