Re: [PATCH] crypto: ccm - avoid scatterlist for MAC encryption

On Sat, 2016-10-15 at 18:16 +0100, Ard Biesheuvel wrote:

The CCM code goes out of its way to perform the CTR encryption of the
MAC using the subordinate CTR driver. To this end, it tweaks the
input and output scatterlists so the aead_req 'odata' and/or
'auth_tag' fields [which may live on the stack] are prepended to the
CTR payload. This involves calling sg_set_buf() on addresses which
are not direct mapped, which is not supported.

Since the calculation of the MAC keystream involves a single call
into the cipher, to which we have a handle already given that the
CBC-MAC calculation uses it as well, just calculate the MAC keystream
directly, and record it in the aead_req private context so we can
apply it to the MAC in cypto_ccm_auth_mac(). This greatly simplifies
the scatterlist manipulation, and no longer requires scatterlists to
refer to buffers that may live on the stack.

No objection from me, Herbert?

I'm getting a bit nervous though - I'd rather have any fix first so
people get things working again - so maybe I'll apply your other patch
and mine first, and then we can replace yours by this later.

johannes

On 17 October 2016 at 08:28, Johannes Berg [off-list ref] wrote:

On Sat, 2016-10-15 at 18:16 +0100, Ard Biesheuvel wrote:

quoted

The CCM code goes out of its way to perform the CTR encryption of the
MAC using the subordinate CTR driver. To this end, it tweaks the
input and output scatterlists so the aead_req 'odata' and/or
'auth_tag' fields [which may live on the stack] are prepended to the
CTR payload. This involves calling sg_set_buf() on addresses which
are not direct mapped, which is not supported.

quoted

Since the calculation of the MAC keystream involves a single call
into the cipher, to which we have a handle already given that the
CBC-MAC calculation uses it as well, just calculate the MAC keystream
directly, and record it in the aead_req private context so we can
apply it to the MAC in cypto_ccm_auth_mac(). This greatly simplifies
the scatterlist manipulation, and no longer requires scatterlists to
refer to buffers that may live on the stack.

No objection from me, Herbert?

I'm getting a bit nervous though - I'd rather have any fix first so
people get things working again - so maybe I'll apply your other patch
and mine first, and then we can replace yours by this later.

Could we get a statement first whether it is supported to allocate
aead_req (and other crypto req structures) on the stack? If not, then
we have our work cut out for us. But if it is, I'd rather we didn't
apply the kzalloc/kfree patch, since it is just a workaround for the
broken generic CCM driver, for which a fix is already available.

Also, regarding your __percpu patch: those are located in the vmalloc
area as well, at least on arm64, and likely other architectures too.

On Mon, Oct 17, 2016 at 12:37 AM, Ard Biesheuvel
[off-list ref] wrote:

On 17 October 2016 at 08:28, Johannes Berg [off-list ref] wrote:

quoted

On Sat, 2016-10-15 at 18:16 +0100, Ard Biesheuvel wrote:

quoted

The CCM code goes out of its way to perform the CTR encryption of the
MAC using the subordinate CTR driver. To this end, it tweaks the
input and output scatterlists so the aead_req 'odata' and/or
'auth_tag' fields [which may live on the stack] are prepended to the
CTR payload. This involves calling sg_set_buf() on addresses which
are not direct mapped, which is not supported.

quoted

Since the calculation of the MAC keystream involves a single call
into the cipher, to which we have a handle already given that the
CBC-MAC calculation uses it as well, just calculate the MAC keystream
directly, and record it in the aead_req private context so we can
apply it to the MAC in cypto_ccm_auth_mac(). This greatly simplifies
the scatterlist manipulation, and no longer requires scatterlists to
refer to buffers that may live on the stack.

No objection from me, Herbert?

I'm getting a bit nervous though - I'd rather have any fix first so
people get things working again - so maybe I'll apply your other patch
and mine first, and then we can replace yours by this later.

Could we get a statement first whether it is supported to allocate
aead_req (and other crypto req structures) on the stack? If not, then
we have our work cut out for us. But if it is, I'd rather we didn't
apply the kzalloc/kfree patch, since it is just a workaround for the
broken generic CCM driver, for which a fix is already available.

I'm not a crypto person, but I don't see why not.  There's even a
helper called SKCIPHER_REQUEST_ON_STACK. :)  The only problem I know
of is pointing a scatterlist at the stack, which is bad for much the
same reason as doing real DMA from the stack.

On 17 October 2016 at 18:08, Andy Lutomirski [off-list ref] wrote:

On Mon, Oct 17, 2016 at 12:37 AM, Ard Biesheuvel
[off-list ref] wrote:

quoted

On 17 October 2016 at 08:28, Johannes Berg [off-list ref] wrote:

quoted

On Sat, 2016-10-15 at 18:16 +0100, Ard Biesheuvel wrote:

quoted

The CCM code goes out of its way to perform the CTR encryption of the
MAC using the subordinate CTR driver. To this end, it tweaks the
input and output scatterlists so the aead_req 'odata' and/or
'auth_tag' fields [which may live on the stack] are prepended to the
CTR payload. This involves calling sg_set_buf() on addresses which
are not direct mapped, which is not supported.

quoted

Since the calculation of the MAC keystream involves a single call
into the cipher, to which we have a handle already given that the
CBC-MAC calculation uses it as well, just calculate the MAC keystream
directly, and record it in the aead_req private context so we can
apply it to the MAC in cypto_ccm_auth_mac(). This greatly simplifies
the scatterlist manipulation, and no longer requires scatterlists to
refer to buffers that may live on the stack.

No objection from me, Herbert?

I'm getting a bit nervous though - I'd rather have any fix first so
people get things working again - so maybe I'll apply your other patch
and mine first, and then we can replace yours by this later.

Could we get a statement first whether it is supported to allocate
aead_req (and other crypto req structures) on the stack? If not, then
we have our work cut out for us. But if it is, I'd rather we didn't
apply the kzalloc/kfree patch, since it is just a workaround for the
broken generic CCM driver, for which a fix is already available.

I'm not a crypto person, but I don't see why not.  There's even a
helper called SKCIPHER_REQUEST_ON_STACK. :)  The only problem I know
of is pointing a scatterlist at the stack, which is bad for much the
same reason as doing real DMA from the stack.

Excellent point!

So the CCM code was an easy fix, although the RFC4309 part is still broken:

It does

"""
scatterwalk_map_and_copy(iv + 16, req->src, 0, req->assoclen - 8, 0);
...
sg_set_buf(rctx->src, iv + 16, req->assoclen - 8);
sg = scatterwalk_ffwd(rctx->src + 1, req->src, req->assoclen);
"""

which essentially just hides the last 8 bytes of associated data from
the inner CCM transform. So we'd need to rewrite this part to create a
new scatterlist that omits those 8 bytes instead of just replacing the
first sg entry and point it to another buffer entirely (and copy the
data into it)

The GCM code is much more complicated, and does not easily allow the
offending sg_set_buf() calls to be turned into something that only
involves direct mapped memory references. I haven't looked at anything
else.

Annoyingly, all this complication with scatterlists etc is for doing
asynchronous crypto via DMA capable crypto accelerators, and the
networking code (ipsec as well as mac80211, afaik) only allow
synchronous in the first place, given that they execute in softirq
context.

On Mon, Oct 17, 2016 at 06:21:14PM +0100, Ard Biesheuvel wrote:

Annoyingly, all this complication with scatterlists etc is for doing
asynchronous crypto via DMA capable crypto accelerators, and the
networking code (ipsec as well as mac80211, afaik) only allow
synchronous in the first place, given that they execute in softirq
context.

I'm still thinking about the issue (in particular, whether we
should continue to rely on the request context being SG-capable
or allow it to be on the stack for AEAD).

But IPsec definitely supports async crypto.  In fact it was the
very first user of async crypto.

mac80211 on the other hand is currently sync-only.

Cheers,
-- 
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On 19 October 2016 at 08:43, Johannes Berg [off-list ref] wrote:

On Wed, 2016-10-19 at 11:31 +0800, Herbert Xu wrote:

quoted

On Mon, Oct 17, 2016 at 06:21:14PM +0100, Ard Biesheuvel wrote:

quoted

Annoyingly, all this complication with scatterlists etc is for
doing
asynchronous crypto via DMA capable crypto accelerators, and the
networking code (ipsec as well as mac80211, afaik) only allow
synchronous in the first place, given that they execute in softirq
context.

I'm still thinking about the issue (in particular, whether we
should continue to rely on the request context being SG-capable
or allow it to be on the stack for AEAD).

:)

quoted

But IPsec definitely supports async crypto.  In fact it was the
very first user of async crypto.

Yeah.

Ah yes, my bad.

quoted

mac80211 on the other hand is currently sync-only.

We could probably make mac80211 do that too, but can we guarantee in-
order processing? Anyway, it's pretty low priority, maybe never
happening, since hardly anyone really uses "software" crypto, the wifi
devices mostly have it built in anyway.

Indeed. The code is now correct in terms of API requirements, so let's
just wait for someone to complain about any performance regressions.

On Wed, 2016-10-19 at 11:31 +0800, Herbert Xu wrote:

On Mon, Oct 17, 2016 at 06:21:14PM +0100, Ard Biesheuvel wrote:

quoted

Annoyingly, all this complication with scatterlists etc is for
doing
asynchronous crypto via DMA capable crypto accelerators, and the
networking code (ipsec as well as mac80211, afaik) only allow
synchronous in the first place, given that they execute in softirq
context.

I'm still thinking about the issue (in particular, whether we
should continue to rely on the request context being SG-capable
or allow it to be on the stack for AEAD).

:)

But IPsec definitely supports async crypto.  In fact it was the
very first user of async crypto.

Yeah.

mac80211 on the other hand is currently sync-only.

We could probably make mac80211 do that too, but can we guarantee in-
order processing? Anyway, it's pretty low priority, maybe never
happening, since hardly anyone really uses "software" crypto, the wifi
devices mostly have it built in anyway.

(One problem is that the skb->cb is already completely full, so we
can't stash away the AAD there)

johannes

On 10/19/2016 08:08 AM, Ard Biesheuvel wrote:

On 19 October 2016 at 08:43, Johannes Berg [off-list ref] wrote:

quoted

On Wed, 2016-10-19 at 11:31 +0800, Herbert Xu wrote:

quoted

We could probably make mac80211 do that too, but can we guarantee in-
order processing? Anyway, it's pretty low priority, maybe never
happening, since hardly anyone really uses "software" crypto, the wifi
devices mostly have it built in anyway.

Indeed. The code is now correct in terms of API requirements, so let's
just wait for someone to complain about any performance regressions.

Do you actually expect performance regressions?  I'll be complaining if
so, but will test first :)

Thanks,
Ben

-- 
Ben Greear [off-list ref]
Candela Technologies Inc  http://www.candelatech.com

On Wed, 2016-10-19 at 08:59 -0700, Ben Greear wrote:

Do you actually expect performance regressions?  I'll be complaining
if so, but will test first :)

I think we can expect this to use a bit more CPU time, but unless
you're very tight on that you probably shouldn't expect any throughput
difference.

johannes