From:   Pascal Van Leeuwen <pvanleeuwen@insidesecure.com>
To:     Eric Biggers <ebiggers@kernel.org>
CC:     "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        Herbert Xu <herbert@gondor.apana.org.au>
Subject: RE: Question regarding crypto scatterlists / testmgr
Thread-Topic: Question regarding crypto scatterlists / testmgr
Thread-Index: AdT1UrNnwXz5dYegQq+/zS0mWvhdgAACM8MAAAEghoAAAaBzgAALBhaA
Date:   Thu, 18 Apr 2019 03:10:35 +0000
Message-ID: <AM6PR09MB352393F7DB634941B4F852FBD2260@AM6PR09MB3523.eurprd09.prod.outlook.com>
References: <AM6PR09MB352386B26E3F632D6FE2FDDDD2250@AM6PR09MB3523.eurprd09.prod.outlook.com>
 <20190417202407.GA96242@gmail.com>
 <AM6PR09MB3523FD88FE64E10B78B555F4D2250@AM6PR09MB3523.eurprd09.prod.outlook.com>
 <20190417214257.GB96242@gmail.com>
In-Reply-To: <20190417214257.GB96242@gmail.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: insidesecure.com does not
 designate permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 85c0d580-fe6b-4c4b-e3e1-08d6c3ab6d36
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Apr 2019 03:10:35.6750
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3c07df58-7760-4e85-afd5-84803eac70ce
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR09MB3416
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

> -----Original Message-----
> From: Eric Biggers [mailto:ebiggers@kernel.org]
> Sent: Wednesday, April 17, 2019 11:43 PM
> To: Pascal Van Leeuwen <pvanleeuwen@insidesecure.com>
> Cc: linux-crypto@vger.kernel.org; Herbert Xu
> <herbert@gondor.apana.org.au>
> Subject: Re: Question regarding crypto scatterlists / testmgr
>
> Hi Pascal,
>
> On Wed, Apr 17, 2019 at 09:16:54PM +0000, Pascal Van Leeuwen wrote:
> > > -----Original Message-----
> > > From: Eric Biggers [mailto:ebiggers@kernel.org]
> > > Sent: Wednesday, April 17, 2019 10:24 PM
> > > To: Pascal Van Leeuwen <pvanleeuwen@insidesecure.com>
> > > Cc: linux-crypto@vger.kernel.org; Herbert Xu
> > > <herbert@gondor.apana.org.au>
> > > Subject: Re: Question regarding crypto scatterlists / testmgr
> > >
> > > Hi Pascal,
> > >
> > > On Wed, Apr 17, 2019 at 07:51:08PM +0000, Pascal Van Leeuwen wrote:
> > > > Hi,
> > > >
> > > > I'm trying to fix the inside-secure driver to pass all testmgr
> > > > tests and I have one final issue remaining with the AEAD ciphers.
> > > > As it was not clear at all what the exact problem was, I spent
> > > > some time reverse engineering testmgr and I got the distinct
> > > > impression that it is using scatter particles that cross page
> > > > boundaries. On purpose, even.
> > > >
> > > > While the inside-secure driver is built on the premise that
> > > > scatter particles are continuous in device space. As I can't
> > > > think of any reason why you would want to scatter/gather other
> > > > than to handle virtual-to-physical address translation ...
> > > > In any case, this should affect all other other operations as
> > > > well, but maybe those just got "lucky" by getting particles
> > > > that were still contiguous in device space, despite the page
> > > > crossing (to *really* verify this, you would have to fully
> > > > randomize your page allocation!)
> > > >
> > > > Anyway, assuming that I *should* be able to handle particles
> > > > that are *not* contiguous in device space, then there should
> > > > probably already exist some function in the kernel API that
> > > > converts a scatterlist with non-contiguous particles into a
> > > > scatterlist with contiguous particles, taking into account the
> > > > presence of an IOMMU? Considering pretty much every device
> > > > driver would need to do that?
> > > > Does anyone know which function(s) to use for that?
> > > >
> > > > Regards,
> > > > Pascal van Leeuwen
> > > > Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
> > > >
> > >
> > > Indeed, since v5.1, testmgr tests scatterlist elements that cross a
> > > page.
> > > However, the pages are guaranteed to be *physically* contiguous.
> Does
> > > dma_map_sg() not handle this?
> > >
> > I'm not entirely sure and the API documentation is not particularly
> > clear on *what* dma_map_sg() actually does, but I highly doubt it
> > considering the particle count is only an input parameter (i.e. it
> > can't output an increase in particles that would be required).
> > So I think it just ensures the pages are actually flushed to memory
> > and accessible by the device (in case an IOMMU interferes) and not
> > much than that.
> >
> > In any case, scatter particles to be used by hardware should *not*
> > cross any physical page boundaries.
> > But also see the thread I had on this with Ard - seems like the
> crypto
> > API already has some mechanism for enforcing this but it's not
> enabled
> > for AEAD ciphers?
> >
> > >
> > > BTW, this isn't just a theoretical case.  Many crypto API users do
> > > crypto on
> > > kmalloced buffers, and those can cross a page boundary, especially
> if
> > > they are
> > > large.  All software crypto algorithms handle this case.
> > >
> > Software sits behind the CPU's MMU and sees virtual memory as
> > contiguous. It does not need to "handle" anything, it gets it for
> free.
> > Hardware does not have that luxury, unless you have a functioning
> IOMMU
> > but that is still pretty rare.
> > So for hardware, you need to break down your buffers until individual
> > pages and stitch those together. That's the main use case of a
> scatter
> > list and it requires the particles to NOT cross physical pages.
> >
> > > The fact that these types of issues are just being considered now
> > > certainly
> > > isn't raising my confidence in the hardware crypto drivers in the
> > > kernel...
> > >
> > Actually, this is *not* a problem with the hardware drivers. It's a
> > problem with the API and/or how you are trying to use it. Hardware
> > does NOT see the nice contiguous virtual memory that SW sees.
> >
>
> I don't understand why you keep talking about virtual memory.  The
> memory in
> each scatterlist element is referenced by struct page, not by virtual
> address.
> It may cross page boundaries; however, all pages referenced by each
> element are
> guaranteed to be adjacent, i.e. physically contiguous.  Am I missing
> something?
>
Ok, I'm not super at home with the behavior of all these kernel API calls
just, just learning, so I did not really know that. I thought you were
trying to say that the pages themselves were contiguous, not that the
pages were contiguous with respect to each other.

The pages not being contiguous seemed to be a perfect explanation for the
behavior I was seeing, and testmgr *does* try to make data cross page
boundaries, which does not seem to be super useful if they're guaranteed
to be contiguous anyway.  However, I just thought of another reason that
could explain the same behavior (and why it affects only AEAD) ... let me
explore that tomorrow.

> Note that memory allocated by kmalloc() is both virtually and
> physically
> contigious.  That's why it works to use sg_init_one() on a kmalloc()'ed
> buffer.
>
> - Eric


Thanks,

Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines