Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAOtvUMeoYcVm7OQdqXd1V5iPSXW_BkVxx6TA6nF7zTLVeHe0Ww@mail.gmail.com>
 <CAOtvUMfy1fF35B2sfbOMui8n9Q4iCke9rgn5TiYMUMjd8gqHsA@mail.gmail.com>
 <YhKV55t90HWm6bhv@Red> <CAOtvUMdRU4wnRCXsC+U5XBDp+b+u8w7W7JCUKW2+ohuJz3PVhQ@mail.gmail.com>
 <YhOcEQEjIKBrbMIZ@Red> <CAOtvUMfN8U4+eG-TEVW4bSE6kOzuOSsJE4dOYGXYuWQKNzv7wQ@mail.gmail.com>
 <CAOtvUMeRb=j=NDrc88x8aB-3=D1mxZ_-aA1d4FfvJmj7Jrbi4w@mail.gmail.com>
 <YiIUXtxd44ut5uzV@Red> <YiUsWosH+MKMF7DQ@gondor.apana.org.au>
 <CAOtvUMcudG3ySU+VeE7hfneDVWGLKFTnws-xjhq4hgFYSj0qOg@mail.gmail.com>
 <YiXjCcXXk0f18FDL@Red> <aca4117c-b7a5-f7eb-eb03-4e1f1a93a730@arm.com>
 <CAOtvUMePFR4e2jgHZKOvs3J3Xt4NzRbzD_=vr_49Qgs5HTrvHw@mail.gmail.com>
 <6cf91f43-df23-3ac9-e9b5-958d99d37422@arm.com> <CAOtvUMedqSNKx9Aah0R_aAyjKO0pn4K75MrCnbh_zX+Zw9vRQA@mail.gmail.com>
 <371ef3f2-883d-91ab-ed96-da8921efb465@arm.com>
In-Reply-To: <371ef3f2-883d-91ab-ed96-da8921efb465@arm.com>
From:   Gilad Ben-Yossef <gilad@benyossef.com>
Date:   Mon, 7 Mar 2022 15:13:05 +0200
Message-ID: <CAOtvUMcp8X9Zex9QE3qz5mKVR9MC3kdUymO1oXVdUc=k1NcwXA@mail.gmail.com>
Subject: Re: [BUG] crypto: ccree: driver does not handle case where cryptlen =
 authsize =0
To:     Robin Murphy <robin.murphy@arm.com>
Cc:     Corentin Labbe <clabbe.montjoie@gmail.com>,
        Christoph Hellwig <hch@lst.de>, m.szyprowski@samsung.com,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Linux kernel mailing list <linux-kernel@vger.kernel.org>,
        iommu@lists.linux-foundation.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Mar 7, 2022 at 3:03 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2022-03-07 12:47, Gilad Ben-Yossef wrote:
> > On Mon, Mar 7, 2022 at 2:36 PM Robin Murphy <robin.murphy@arm.com> wrot=
e:
> >>
> >> On 2022-03-07 12:17, Gilad Ben-Yossef wrote:
> >>> On Mon, Mar 7, 2022 at 1:14 PM Robin Murphy <robin.murphy@arm.com> wr=
ote:
> >>>
> >>>> The "overlap" is in the sense of having more than one mapping within=
 the
> >>>> same cacheline:
> >>>>
> >>>> [  142.458120] DMA-API: add_dma_entry start P=3Dba79f200 N=3Dba79f
> >>>> D=3Dba79f200 L=3D10 DMA_FROM_DEVICE attrs=3D0
> >>>> [  142.458156] DMA-API: add_dma_entry start P=3D445dc010 N=3D445dc
> >>>> D=3D445dc010 L=3D10 DMA_TO_DEVICE attrs=3D0
> >>>> [  142.458178] sun8i-ss 1c15000.crypto: SRC 0/1/1 445dc000 len=3D16 =
bi=3D0
> >>>> [  142.458215] sun8i-ss 1c15000.crypto: DST 0/1/1 ba79f200 len=3D16 =
bi=3D0
> >>>> [  142.458234] DMA-API: add_dma_entry start P=3Dba79f210 N=3Dba79f
> >>>> D=3Dba79f210 L=3D10 DMA_FROM_DEVICE attrs=3D0
> >>>>
> >>>> This actually illustrates exactly the reason why this is unsupportab=
le.
> >>>> ba79f200 is mapped for DMA_FROM_DEVICE, therefore subsequently mappi=
ng
> >>>> ba79f210 for DMA_TO_DEVICE may cause the cacheline covering the rang=
e
> >>>> ba79f200-ba79f23f to be written back over the top of data that the
> >>>> device has already started to write to memory. Hello data corruption=
.
> >>>>
> >>>> Separate DMA mappings should be from separate memory allocations,
> >>>> respecting ARCH_DMA_MINALIGN.
> >>>
> >>> hmm... I know I'm missing something here, but how does this align wit=
h
> >>> the following from active_cacheline_insert() in kernel/dma/debug.c ?
> >>>
> >>>           /* If the device is not writing memory then we don't have a=
ny
> >>>            * concerns about the cpu consuming stale data.  This mitig=
ates
> >>>            * legitimate usages of overlapping mappings.
> >>>            */
> >>>           if (entry->direction =3D=3D DMA_TO_DEVICE)
> >>>                   return 0;
> >>
> >> It's OK to have multiple mappings that are *all* DMA_TO_DEVICE, which
> >> looks to be the case that this check was intended to allow. However I
> >> think you're right that it should still actually check for conflicting
> >> directions between the new entry and any existing ones, otherwise it
> >> ends up a bit too lenient.
> >>
> >> Cheers,
> >> Robin.
> >
> > I understand what you are saying about why checking for conflicting
> > directions may be a good thing, but given that the code is as it is
> > right now, how are we seeing the warning for two mapping that one of
> > them is DMA_TO_DEVICE?
>
> Because it's the second one that isn't. The warning is triggered by
> adding the DMA_FROM_DEVICE entry, which *is* checked, and finds the
> DMA_TO_DEVICE entry already present. What's not great is that if those
> two mappings happened to be made in the opposite order then it would be
> missed entirely.

Please accept my sincere apologies if I'm being daft , but here is the
code for add_dma_entry():

static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attr=
s)
{
        struct hash_bucket *bucket;
        unsigned long flags;
        int rc;

        bucket =3D get_hash_bucket(entry, &flags);
        hash_bucket_add(bucket, entry);
        put_hash_bucket(bucket, flags);

        rc =3D active_cacheline_insert(entry);
        if (rc =3D=3D -ENOMEM) {
                pr_err("cacheline tracking ENOMEM, dma-debug disabled\n");
                global_disable =3D true;
        } else if (rc =3D=3D -EEXIST && !(attrs & DMA_ATTR_SKIP_CPU_SYNC)) =
{
                err_printk(entry->dev, entry,
                        "cacheline tracking EEXIST, overlapping
mappings aren't supported\n");
        }
}

Clearly we get to active_cacheline_insert() unconditionally.

Here is the code of active_cacheline_insert():

static int active_cacheline_insert(struct dma_debug_entry *entry)
{
        phys_addr_t cln =3D to_cacheline_number(entry);
        unsigned long flags;
        int rc;

        /* If the device is not writing memory then we don't have any
         * concerns about the cpu consuming stale data.  This mitigates
         * legitimate usages of overlapping mappings.
         */
        if (entry->direction =3D=3D DMA_TO_DEVICE)
                return 0;

        spin_lock_irqsave(&radix_lock, flags);
        rc =3D radix_tree_insert(&dma_active_cacheline, cln, entry);
        if (rc =3D=3D -EEXIST)
                active_cacheline_inc_overlap(cln);
        spin_unlock_irqrestore(&radix_lock, flags);

        return rc;
}

Clearly the check for direction happens BEFORE we ever attempt to add
the cacheline tracking data.

So it shouldn't matter at all which is first and which is second... :-(

I know I'm missing something. But what?

Thanks,
Gilad

--=20
Gilad Ben-Yossef
Chief Coffee Drinker

values of =CE=B2 will give rise to dom!