* Re: x86-64: Maintain 16-byte stack alignment
From: Josh Poimboeuf @ 2017-01-12 14:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Herbert Xu, Linus Torvalds, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <CALCETrVb-NyKJajGEsLOrQqQrQ7cRm4sqLRxNr_yVE2-mjiDrg@mail.gmail.com>
On Wed, Jan 11, 2017 at 10:21:07PM -0800, Andy Lutomirski wrote:
> On Tue, Jan 10, 2017 at 10:01 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Tue, Jan 10, 2017 at 8:35 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >> On Tue, Jan 10, 2017 at 08:17:17PM -0800, Linus Torvalds wrote:
> >>>
> >>> That said, I do think that the "don't assume stack alignment, do it by
> >>> hand" may be the safer thing. Because who knows what the random rules
> >>> will be on other architectures.
> >>
> >> Sure we can ban the use of attribute aligned on stacks. But
> >> what about indirect uses through structures? For example, if
> >> someone does
> >>
> >> struct foo {
> >> } __attribute__ ((__aligned__(16)));
> >>
> >> int bar(...)
> >> {
> >> struct foo f;
> >>
> >> return baz(&f);
> >> }
> >>
> >> then baz will end up with an unaligned argument. The worst part
> >> is that it is not at all obvious to the person writing the function
> >> bar.
> >
> > Linus, I'm starting to lean toward agreeing with Herbert here, except
> > that we should consider making it conditional on having a silly GCC
> > version. After all, the silly GCC versions are wasting space and time
> > with alignment instructions no matter what we do, so this would just
> > mean tweaking the asm and adding some kind of check_stack_alignment()
> > helper to throw out a WARN_ONCE() if we miss one. The problem with
> > making it conditional is that making pt_regs effectively live at a
> > variable offset from %rsp is just nasty.
>
> So actually doing this is gross because we have calls from asm to C
> all over the place. But... maybe we can automate all the testing.
> Josh, how hard would it be to teach objtool to (if requested by an
> option) check that stack frames with statically known size preserve
> 16-byte stack alignment?
>
> I find it rather annoying that gcc before 4.8 malfunctions when it
> sees __aligned__(16) on x86_64 kernels. Sigh.
Just to clarify, I think you're asking if, for versions of gcc which
don't support -mpreferred-stack-boundary=3, objtool can analyze all C
functions to ensure their stacks are 16-byte aligned.
It's certainly possible, but I don't see how that solves the problem.
The stack will still be misaligned by entry code. Or am I missing
something?
--
Josh
^ permalink raw reply
* Re: [PATCH v4 3/3] drivers: crypto: Enable CPT options crypto for build
From: kbuild test robot @ 2017-01-12 14:04 UTC (permalink / raw)
To: George Cherian
Cc: kbuild-all, herbert, davem, david.daney, clabbe.montjoie,
linux-kernel, linux-crypto, George Cherian
In-Reply-To: <1484132211-917-4-git-send-email-george.cherian@cavium.com>
[-- Attachment #1: Type: text/plain, Size: 3398 bytes --]
Hi George,
[auto build test WARNING on v4.9-rc8]
[cannot apply to cryptodev/master crypto/master sparc-next/master next-20170111]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/George-Cherian/Add-Support-for-Cavium-Cryptographic-Acceleration-Unit/20170112-192240
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=parisc
All warnings (new ones prefixed by >>):
drivers/crypto/cavium/cpt/cptvf_reqmanager.c: In function 'process_request':
>> drivers/crypto/cavium/cpt/cptvf_reqmanager.c:470:49: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'unsigned int' [-Wformat=]
dev_err(&pdev->dev, "mapping compptr Failed %lu\n",
^
vim +470 drivers/crypto/cavium/cpt/cptvf_reqmanager.c
8413476d George Cherian 2017-01-11 454 goto request_cleanup;
8413476d George Cherian 2017-01-11 455 }
8413476d George Cherian 2017-01-11 456
8413476d George Cherian 2017-01-11 457 cpt_req->dlen = info->dlen;
8413476d George Cherian 2017-01-11 458 /*
8413476d George Cherian 2017-01-11 459 * Get buffer for union cpt_res_s response
8413476d George Cherian 2017-01-11 460 * structure and its physical address
8413476d George Cherian 2017-01-11 461 */
8413476d George Cherian 2017-01-11 462 info->completion_addr = kzalloc(sizeof(union cpt_res_s),
8413476d George Cherian 2017-01-11 463 GFP_KERNEL | GFP_ATOMIC);
8413476d George Cherian 2017-01-11 464 *((u8 *)(info->completion_addr)) = COMPLETION_CODE_INIT;
8413476d George Cherian 2017-01-11 465 info->comp_baddr = dma_map_single(&pdev->dev,
8413476d George Cherian 2017-01-11 466 (void *)info->completion_addr,
8413476d George Cherian 2017-01-11 467 sizeof(union cpt_res_s),
8413476d George Cherian 2017-01-11 468 DMA_BIDIRECTIONAL);
8413476d George Cherian 2017-01-11 469 if (dma_mapping_error(&pdev->dev, info->comp_baddr)) {
8413476d George Cherian 2017-01-11 @470 dev_err(&pdev->dev, "mapping compptr Failed %lu\n",
8413476d George Cherian 2017-01-11 471 sizeof(union cpt_res_s));
8413476d George Cherian 2017-01-11 472 ret = -EFAULT;
8413476d George Cherian 2017-01-11 473 goto request_cleanup;
8413476d George Cherian 2017-01-11 474 }
8413476d George Cherian 2017-01-11 475
8413476d George Cherian 2017-01-11 476 /* Fill the VQ command */
8413476d George Cherian 2017-01-11 477 vq_cmd.cmd.u64 = 0;
8413476d George Cherian 2017-01-11 478 vq_cmd.cmd.s.opcode = cpu_to_be16(cpt_req->opcode.flags);
:::::: The code at line 470 was first introduced by commit
:::::: 8413476deed83359518ea36cc316f4669a8c521c drivers: crypto: Add the Virtual Function driver for CPT
:::::: TO: George Cherian <george.cherian@cavium.com>
:::::: CC: 0day robot <fengguang.wu@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 47592 bytes --]
^ permalink raw reply
* Re: [PATCH v8 1/1] crypto: add virtio-crypto driver
From: Christian Borntraeger @ 2017-01-12 14:10 UTC (permalink / raw)
To: Gonglei (Arei), linux-kernel@vger.kernel.org,
qemu-devel@nongnu.org, virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org,
linux-crypto@vger.kernel.org, davem@davemloft.net,
herbert@gondor.apana.org.au
Cc: Huangweidong (C), Claudio Fontana, mst@redhat.com, Luonengjun,
Hanweidong (Randy), Xuquan (Quan Xu), Wanzongshun (Vincent),
stefanha@redhat.com, Zhoujian (jay, Euler), longpeng,
arei.gonglei@hotmail.com, Wubin (H)
In-Reply-To: <c4282723-2afb-1764-2431-ef41f7126371@de.ibm.com>
On 01/10/2017 01:56 PM, Christian Borntraeger wrote:
> On 01/10/2017 01:36 PM, Gonglei (Arei) wrote:
>> Hi,
>>
>>>
>>> On 12/15/2016 03:03 AM, Gonglei wrote:
>>> [...]
>>>> +
>>>> +static struct crypto_alg virtio_crypto_algs[] = { {
>>>> + .cra_name = "cbc(aes)",
>>>> + .cra_driver_name = "virtio_crypto_aes_cbc",
>>>> + .cra_priority = 501,
>>>
>>>
>>> This is still higher than the hardware-accelerators (like intel aesni or the
>>> s390 cpacf functions or the arm hw). aesni and s390/cpacf are supported by the
>>> hardware virtualization and available to the guests. I do not see a way how
>>> virtio
>>> crypto can be faster than that (in the end it might be cpacf/aesni + overhead)
>>> instead it will very likely be slower.
>>> So we should use a number that is higher than software implementations but
>>> lower than the hw ones.
>>>
>>> Just grepping around, the software ones seem be be around 100 and the
>>> hardware
>>> ones around 200-400. So why was 150 not enough?
>>>
>> I didn't find a documentation about how we use the priority, and I assumed
>> people use virtio-crypto will configure hardware accelerators in the
>> host. So I choosed the number which bigger than aesni's priority.
>
> Yes, but the aesni driver will only bind if there is HW support in the guest.
> And if aesni is available in the guest (or the s390 aes function from cpacf)
> it will always be faster than the same in the host via virtio.So your priority
> should be smaller.
any opinion on this?
^ permalink raw reply
* Re: [PATCH v8 1/1] crypto: add virtio-crypto driver
From: Michael S. Tsirkin @ 2017-01-12 14:23 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Gonglei (Arei), linux-kernel@vger.kernel.org,
qemu-devel@nongnu.org, virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org,
linux-crypto@vger.kernel.org, davem@davemloft.net,
herbert@gondor.apana.org.au, Huangweidong (C), Claudio Fontana,
Luonengjun, Hanweidong (Randy), Xuquan (Quan Xu),
Wanzongshun (Vincent), "stefanha@redhat.com" <ste
In-Reply-To: <a8384ff8-ff65-c578-c338-e6d8ebf498c4@de.ibm.com>
On Thu, Jan 12, 2017 at 03:10:25PM +0100, Christian Borntraeger wrote:
> On 01/10/2017 01:56 PM, Christian Borntraeger wrote:
> > On 01/10/2017 01:36 PM, Gonglei (Arei) wrote:
> >> Hi,
> >>
> >>>
> >>> On 12/15/2016 03:03 AM, Gonglei wrote:
> >>> [...]
> >>>> +
> >>>> +static struct crypto_alg virtio_crypto_algs[] = { {
> >>>> + .cra_name = "cbc(aes)",
> >>>> + .cra_driver_name = "virtio_crypto_aes_cbc",
> >>>> + .cra_priority = 501,
> >>>
> >>>
> >>> This is still higher than the hardware-accelerators (like intel aesni or the
> >>> s390 cpacf functions or the arm hw). aesni and s390/cpacf are supported by the
> >>> hardware virtualization and available to the guests. I do not see a way how
> >>> virtio
> >>> crypto can be faster than that (in the end it might be cpacf/aesni + overhead)
> >>> instead it will very likely be slower.
> >>> So we should use a number that is higher than software implementations but
> >>> lower than the hw ones.
> >>>
> >>> Just grepping around, the software ones seem be be around 100 and the
> >>> hardware
> >>> ones around 200-400. So why was 150 not enough?
> >>>
> >> I didn't find a documentation about how we use the priority, and I assumed
> >> people use virtio-crypto will configure hardware accelerators in the
> >> host. So I choosed the number which bigger than aesni's priority.
> >
> > Yes, but the aesni driver will only bind if there is HW support in the guest.
> > And if aesni is available in the guest (or the s390 aes function from cpacf)
> > it will always be faster than the same in the host via virtio.So your priority
> > should be smaller.
>
>
> any opinion on this?
Going forward, we might add an emulated aesni device and that might
become slower than virtio. OTOH if or when this happens, we can solve it
by adding a priority or a feature flag to virtio to raise its priority.
So I think I agree with Christian here, let's lower the priority.
Gonglei, could you send a patch like this?
--
MST
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Josh Poimboeuf @ 2017-01-12 14:49 UTC (permalink / raw)
To: Ingo Molnar
Cc: Herbert Xu, Linus Torvalds, Linux Kernel Mailing List,
Linux Crypto Mailing List, Thomas Gleixner, Andy Lutomirski,
Ard Biesheuvel
In-Reply-To: <20170112074601.GB30151@gmail.com>
On Thu, Jan 12, 2017 at 08:46:01AM +0100, Ingo Molnar wrote:
>
> * Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> > On Tue, Jan 10, 2017 at 09:05:28AM -0800, Linus Torvalds wrote:
> > >
> > > I'm pretty sure we have random asm code that may not maintain a
> > > 16-byte stack alignment when it calls other code (including, in some
> > > cases, calling C code).
> > >
> > > So I'm not at all convinced that this is a good idea. We shouldn't
> > > expect 16-byte alignment to be something trustworthy.
> >
> > So what if we audited all the x86 assembly code to fix this? Would
> > it then be acceptable to do a 16-byte aligned stack?
>
> Audits for small but deadly details that isn't checked automatically by tooling
> would inevitably bitrot again - and in this particular case there's a 50% chance
> that a new, buggy change would test out to be 'fine' on a kernel developer's own
> box - and break on different configs, different hw or with unrelated (and
> innocent) kernel changes, sometime later - spreading the pain unnecessarily.
>
> So my feeling is that we really need improved tooling for this (and yes, the GCC
> toolchain should have handled this correctly).
>
> But fortunately we have related tooling in the kernel: could objtool handle this?
> My secret hope was always that objtool would grow into a kind of life insurance
> against toolchain bogosities (which is a must for things like livepatching or a
> DWARF unwinder - but I digress).
Are we talking about entry code, or other asm code? Because objtool
audits *most* asm code, but entry code is way too specialized for
objtool to understand.
(I do have a pending objtool rewrite which would make it very easy to
ensure 16-byte stack alignment. But again, objtool can only understand
callable C or asm functions, not entry code.)
Another approach would be to solve this problem with unwinder warnings,
*if* there's enough test coverage.
I recently made some changes to try to standardize the "end" of the
stack, so that the stack pointer is always a certain value before
calling into C code. I also added some warnings to the unwinder to
ensure that it always reaches that point on the stack. So if the "end"
of the stack were adjusted by a word by adding padding to pt_regs, the
unwinder warnings could help preserve that.
We could take that a step further by adding an unwinder check to ensure
that *every* frame is 16-byte aligned if -mpreferred-stack-boundary=3
isn't used.
Yet another step would be to add a debug feature which does stack sanity
checking from a periodic NMI, to flush out these unwinder warnings.
(Though I've found that current 0-day and fuzzing efforts, combined with
lockdep and perf's frequent unwinder usage, are already doing a great
job at flushing out unwinder warnings.)
The only question is if there would be enough test coverage,
particularly with those versions of gcc which don't have
-mpreferred-stack-boundary=3.
--
Josh
^ permalink raw reply
* Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
From: Herbert Xu @ 2017-01-12 14:57 UTC (permalink / raw)
To: Stephan Müller; +Cc: linux-crypto
In-Reply-To: <31581301.kpbcu3havF@positron.chronox.de>
On Thu, Jan 12, 2017 at 12:22:09PM +0100, Stephan Müller wrote:
>
> When addressing the issue in the algif_aead code, and expect that over time
> the AEAD implementations will gain the copy operation, eventually we will copy
> the AAD twice. Of course, this could be prevented, if the algif_aead code
> somehow uses the same SGL for the src and dst AAD.
Why would you copy it twice? You copy everything before you start
and then just do in-place crypto.
> > BTW, why are you only doing the copy for encryption?
>
> I was looking at the only AEAD implementation that does the copy operation:
> authenc. There, the copy operation is only performed for encryption. I was
> thinking a bit about why decryption was not covered. I think the answer is the
> following: for encryption, the AAD is definitely needed in the dst buffer as
> the dst buffer with the AAD must be sent to the recipient for decryption. The
> decryption and the associated authentication only works with the AAD. However,
> after decrypting, all the caller wants is the decrypted plaintext only. There
> is no further use of the AAD after the decryption step. Hence, copying the AAD
> to the dst buffer in the decryption step would not serve the caller.
That's just the current implementation. If we're going to make
this an API then we should do it for both directions.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Josh Poimboeuf @ 2017-01-12 15:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Herbert Xu, Linus Torvalds, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <CALCETrV78gNs5ZqunaSPZ4AATapb1+ESKd6Wv+5rdMpZe9TSJQ@mail.gmail.com>
On Wed, Jan 11, 2017 at 11:51:10PM -0800, Andy Lutomirski wrote:
> On Wed, Jan 11, 2017 at 11:05 PM, Herbert Xu
> <herbert@gondor.apana.org.au> wrote:
> > On Tue, Jan 10, 2017 at 09:05:28AM -0800, Linus Torvalds wrote:
> >>
> >> I'm pretty sure we have random asm code that may not maintain a
> >> 16-byte stack alignment when it calls other code (including, in some
> >> cases, calling C code).
> >>
> >> So I'm not at all convinced that this is a good idea. We shouldn't
> >> expect 16-byte alignment to be something trustworthy.
> >
> > So what if we audited all the x86 assembly code to fix this? Would
> > it then be acceptable to do a 16-byte aligned stack?
> >
> > On the face of it it doesn't seem to be a huge amount of code
> > assuming they mostly live under arch/x86.
>
> The problem is that we have nasties like TRACE_IRQS_OFF. Performance
> doesn't really matter for these macros, so we could probably rig up a
> helper for forcibly align the stack there. Maybe
> FRAME_BEGIN_FORCE_ALIGN? I also think I'd rather not to modify
> pt_regs. We should just fix the small number of code paths that
> create a pt_regs and then call into C code to align the stack.
>
> But if we can't do this with automatic verification, then I'm not sure
> I want to do it at all. The asm is already more precarious than I'd
> like, and having a code path that is misaligned is asking for obscure
> bugs down the road.
For the entry code, could we just replace all calls with CALL_ALIGNED?
That might be less intrusive than trying to adjust all the pt_regs
accesses.
Then to ensure that nobody ever uses 'call' directly:
'#define call please-use-CALL-ALIGNED-instead-of-call'
I think that would be possible if CALL_ALIGNED were a ".macro".
--
Josh
^ permalink raw reply
* [PATCH -next] crypto: mediatek - make symbol of_crypto_id static
From: Wei Yongjun @ 2017-01-12 15:03 UTC (permalink / raw)
To: Herbert Xu, Matthias Brugger, Ryder Lee
Cc: linux-mediatek, Wei Yongjun, linux-crypto, linux-arm-kernel
From: Wei Yongjun <weiyongjun1@huawei.com>
Fixes the following sparse warning:
drivers/crypto/mediatek/mtk-platform.c:585:27: warning:
symbol 'of_crypto_id' was not declared. Should it be static?
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
drivers/crypto/mediatek/mtk-platform.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/mediatek/mtk-platform.c b/drivers/crypto/mediatek/mtk-platform.c
index 286296f..a9c713d 100644
--- a/drivers/crypto/mediatek/mtk-platform.c
+++ b/drivers/crypto/mediatek/mtk-platform.c
@@ -582,7 +582,7 @@ static int mtk_crypto_remove(struct platform_device *pdev)
return 0;
}
-const struct of_device_id of_crypto_id[] = {
+static const struct of_device_id of_crypto_id[] = {
{ .compatible = "mediatek,eip97-crypto" },
{},
};
^ permalink raw reply related
* Re: x86-64: Maintain 16-byte stack alignment
From: Herbert Xu @ 2017-01-12 15:06 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Andy Lutomirski, Linus Torvalds, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <20170112150318.eq4lmfitxvitpix4@treble>
On Thu, Jan 12, 2017 at 09:03:18AM -0600, Josh Poimboeuf wrote:
>
> For the entry code, could we just replace all calls with CALL_ALIGNED?
> That might be less intrusive than trying to adjust all the pt_regs
> accesses.
>
> Then to ensure that nobody ever uses 'call' directly:
>
> '#define call please-use-CALL-ALIGNED-instead-of-call'
>
> I think that would be possible if CALL_ALIGNED were a ".macro".
The trouble with that is that you have got things like TRACE_IRQS_OFF
which are also used outside of the entry code.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Josh Poimboeuf @ 2017-01-12 15:10 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Herbert Xu, Linus Torvalds, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <20170112150318.eq4lmfitxvitpix4@treble>
On Thu, Jan 12, 2017 at 09:03:18AM -0600, Josh Poimboeuf wrote:
> On Wed, Jan 11, 2017 at 11:51:10PM -0800, Andy Lutomirski wrote:
> > On Wed, Jan 11, 2017 at 11:05 PM, Herbert Xu
> > <herbert@gondor.apana.org.au> wrote:
> > > On Tue, Jan 10, 2017 at 09:05:28AM -0800, Linus Torvalds wrote:
> > >>
> > >> I'm pretty sure we have random asm code that may not maintain a
> > >> 16-byte stack alignment when it calls other code (including, in some
> > >> cases, calling C code).
> > >>
> > >> So I'm not at all convinced that this is a good idea. We shouldn't
> > >> expect 16-byte alignment to be something trustworthy.
> > >
> > > So what if we audited all the x86 assembly code to fix this? Would
> > > it then be acceptable to do a 16-byte aligned stack?
> > >
> > > On the face of it it doesn't seem to be a huge amount of code
> > > assuming they mostly live under arch/x86.
> >
> > The problem is that we have nasties like TRACE_IRQS_OFF. Performance
> > doesn't really matter for these macros, so we could probably rig up a
> > helper for forcibly align the stack there. Maybe
> > FRAME_BEGIN_FORCE_ALIGN? I also think I'd rather not to modify
> > pt_regs. We should just fix the small number of code paths that
> > create a pt_regs and then call into C code to align the stack.
> >
> > But if we can't do this with automatic verification, then I'm not sure
> > I want to do it at all. The asm is already more precarious than I'd
> > like, and having a code path that is misaligned is asking for obscure
> > bugs down the road.
>
> For the entry code, could we just replace all calls with CALL_ALIGNED?
> That might be less intrusive than trying to adjust all the pt_regs
> accesses.
>
> Then to ensure that nobody ever uses 'call' directly:
>
> '#define call please-use-CALL-ALIGNED-instead-of-call'
>
> I think that would be possible if CALL_ALIGNED were a ".macro".
To clarify, CALL_ALIGNED could be (completely untested):
.macro CALL_ALIGNED \func
push %rbp
movq %rsp, %rbp
and $0xfffffffffffffff0,%rsp
call \func
movq %rbp, %rsp
pop %rbp
.endm
--
Josh
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Josh Poimboeuf @ 2017-01-12 15:18 UTC (permalink / raw)
To: Herbert Xu
Cc: Andy Lutomirski, Linus Torvalds, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <20170112150650.GA16000@gondor.apana.org.au>
On Thu, Jan 12, 2017 at 11:06:50PM +0800, Herbert Xu wrote:
> On Thu, Jan 12, 2017 at 09:03:18AM -0600, Josh Poimboeuf wrote:
> >
> > For the entry code, could we just replace all calls with CALL_ALIGNED?
> > That might be less intrusive than trying to adjust all the pt_regs
> > accesses.
> >
> > Then to ensure that nobody ever uses 'call' directly:
> >
> > '#define call please-use-CALL-ALIGNED-instead-of-call'
> >
> > I think that would be possible if CALL_ALIGNED were a ".macro".
>
> The trouble with that is that you have got things like TRACE_IRQS_OFF
> which are also used outside of the entry code.
Where? As far as I can tell, TRACE_IRQS_OFF is used exclusively by entry
code.
--
Josh
^ permalink raw reply
* Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
From: Stephan Müller @ 2017-01-12 15:23 UTC (permalink / raw)
To: Herbert Xu; +Cc: linux-crypto
In-Reply-To: <20170112145728.GA15595@gondor.apana.org.au>
Am Donnerstag, 12. Januar 2017, 22:57:28 CET schrieb Herbert Xu:
Hi Herbert,
> On Thu, Jan 12, 2017 at 12:22:09PM +0100, Stephan Müller wrote:
> > When addressing the issue in the algif_aead code, and expect that over
> > time
> > the AEAD implementations will gain the copy operation, eventually we will
> > copy the AAD twice. Of course, this could be prevented, if the algif_aead
> > code somehow uses the same SGL for the src and dst AAD.
>
> Why would you copy it twice? You copy everything before you start
> and then just do in-place crypto.
>
As far as I understand, we have the following AAD copy operations that we
discuss:
- algif_aead: you suggested to add the AAD copy operation here
- AEAD implementations: over time, the AEAD implementations shall receive the
AAD copy operation. The AAD copy operation shall only take place if the src
SGL != dst SGL
The algif_aead code *always* will have the src SGL being different from the
dst SGL. Thus, any existing AEAD implementation copy will always be performed
which would be in addition to the algif_aead AAD copy operation. We can only
avoid the AEAD implementation copy operation, if we add some code to
algif_aead to make sure that the AAD data is pointed to by src/dst SGL that is
identical. However, such logic to make src/dst SGL identical is quite complex
compared to simply use one callback as suggested in the current patch set.
In the followup email, I suggested to add the AAD copy function invocation
into crypto_aead_encrypt. This way, we can drop the numerous patches to the
AEAD implementations and yet we can avoid adding such copy operation and src/
dst SGL unification logic to algif_aead.
> > > BTW, why are you only doing the copy for encryption?
> >
> > I was looking at the only AEAD implementation that does the copy
> > operation:
> > authenc. There, the copy operation is only performed for encryption. I was
> > thinking a bit about why decryption was not covered. I think the answer is
> > the following: for encryption, the AAD is definitely needed in the dst
> > buffer as the dst buffer with the AAD must be sent to the recipient for
> > decryption. The decryption and the associated authentication only works
> > with the AAD. However, after decrypting, all the caller wants is the
> > decrypted plaintext only. There is no further use of the AAD after the
> > decryption step. Hence, copying the AAD to the dst buffer in the
> > decryption step would not serve the caller.
> That's just the current implementation. If we're going to make
> this an API then we should do it for both directions.
Considering the suggestion above to add the AAD copy call to
crypto_aead_encrypt, we can add such copy call also to crypto_aead_decrypt.
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
From: Herbert Xu @ 2017-01-12 15:27 UTC (permalink / raw)
To: Stephan Müller; +Cc: linux-crypto
In-Reply-To: <6428553.skGSKSKcYl@tauon.atsec.com>
On Thu, Jan 12, 2017 at 04:23:50PM +0100, Stephan Müller wrote:
>
> As far as I understand, we have the following AAD copy operations that we
> discuss:
>
> - algif_aead: you suggested to add the AAD copy operation here
>
> - AEAD implementations: over time, the AEAD implementations shall receive the
> AAD copy operation. The AAD copy operation shall only take place if the src
> SGL != dst SGL
If and when that happens we'd simply need to remove the copy from
algif_aead code. But even if we didn't you wouldn't have two copies
because algif_aead should invoke the in-place code-path after doing
a full copy of src to dst.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
From: Stephan Müller @ 2017-01-12 15:34 UTC (permalink / raw)
To: Herbert Xu; +Cc: linux-crypto
In-Reply-To: <20170112152710.GA16222@gondor.apana.org.au>
Am Donnerstag, 12. Januar 2017, 23:27:10 CET schrieb Herbert Xu:
Hi Herbert,
> On Thu, Jan 12, 2017 at 04:23:50PM +0100, Stephan Müller wrote:
> > As far as I understand, we have the following AAD copy operations that we
> > discuss:
> >
> > - algif_aead: you suggested to add the AAD copy operation here
> >
> > - AEAD implementations: over time, the AEAD implementations shall receive
> > the AAD copy operation. The AAD copy operation shall only take place if
> > the src SGL != dst SGL
>
> If and when that happens we'd simply need to remove the copy from
> algif_aead code.
We would only be able to remove it if all AEAD implementations are converted.
But for the conversion time, we do face that issue.
> But even if we didn't you wouldn't have two copies
> because algif_aead should invoke the in-place code-path after doing
> a full copy of src to dst.
Are you suggesting that the entire data in the src SGL is first copied to the
dst SGL by algif_aead? If yes, that still requires significant src/dst SGL
tinkering as we have the tag -- the src SGL for encrypt does not have the tag
space where the dst SGL for encrypt is required to have the tag size. This is
vice versa for the decryption operation.
And to me the most elegant solution seems adding the copy operation to
crypto_aead_[en|de]crypt.
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
From: Herbert Xu @ 2017-01-12 15:39 UTC (permalink / raw)
To: Stephan Müller; +Cc: linux-crypto
In-Reply-To: <2819680.QEYfZCVF62@tauon.atsec.com>
On Thu, Jan 12, 2017 at 04:34:39PM +0100, Stephan Müller wrote:
>
> We would only be able to remove it if all AEAD implementations are converted.
> But for the conversion time, we do face that issue.
It doesn't matter. Nobody in the kernel uses that. In fact I
wonder whether we should even do it for the kernel API. We only
need it for the user-space API because it goes through read/write.
> Are you suggesting that the entire data in the src SGL is first copied to the
> dst SGL by algif_aead? If yes, that still requires significant src/dst SGL
> tinkering as we have the tag -- the src SGL for encrypt does not have the tag
> space where the dst SGL for encrypt is required to have the tag size. This is
> vice versa for the decryption operation.
It's really only a problem for decryption. In that case you can
extend the dst SG list to include the tag.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] crypto: kpp - clear CRYPTO_ALG_DEAD bit in prepare_alg
From: Herbert Xu @ 2017-01-12 15:41 UTC (permalink / raw)
To: Salvatore Benedetto; +Cc: linux-crypto
In-Reply-To: <1483364021-103771-1-git-send-email-salvatore.benedetto@intel.com>
On Mon, Jan 02, 2017 at 01:33:41PM +0000, Salvatore Benedetto wrote:
> Make sure CRYPTO_ALG_DEAD is not set when preparing for
> alg registration. This fixes qat-dh registration that occurs
> when reloading qat_c62x module.
>
> Signed-off-by: Salvatore Benedetto <salvatore.benedetto@intel.com>
> ---
> crypto/kpp.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/crypto/kpp.c b/crypto/kpp.c
> index d36ce05..d1adef8e 100644
> --- a/crypto/kpp.c
> +++ b/crypto/kpp.c
> @@ -101,6 +101,7 @@ static void kpp_prepare_alg(struct kpp_alg *alg)
>
> base->cra_type = &crypto_kpp_type;
> base->cra_flags &= ~CRYPTO_ALG_TYPE_MASK;
> + base->cra_flags &= ~CRYPTO_ALG_DEAD;
> base->cra_flags |= CRYPTO_ALG_TYPE_KPP;
> }
Same comment as in
https://patchwork.kernel.org/patch/9485115/
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
From: Stephan Müller @ 2017-01-12 15:50 UTC (permalink / raw)
To: Herbert Xu; +Cc: linux-crypto
In-Reply-To: <20170112153924.GA16347@gondor.apana.org.au>
Am Donnerstag, 12. Januar 2017, 23:39:24 CET schrieb Herbert Xu:
Hi Herbert,
> On Thu, Jan 12, 2017 at 04:34:39PM +0100, Stephan Müller wrote:
> > We would only be able to remove it if all AEAD implementations are
> > converted. But for the conversion time, we do face that issue.
>
> It doesn't matter. Nobody in the kernel uses that. In fact I
> wonder whether we should even do it for the kernel API. We only
> need it for the user-space API because it goes through read/write.
So you say that we could remove it from authenc() entirely (this is currently
the only location where such copy operation is found for the encryption
direction)?
I would concur that the kernel does not need that.
>
> > Are you suggesting that the entire data in the src SGL is first copied to
> > the dst SGL by algif_aead? If yes, that still requires significant
> > src/dst SGL tinkering as we have the tag -- the src SGL for encrypt does
> > not have the tag space where the dst SGL for encrypt is required to have
> > the tag size. This is vice versa for the decryption operation.
>
> It's really only a problem for decryption. In that case you can
> extend the dst SG list to include the tag.
If we only want to solve that for algif_aead, wouldn't it make more sense if
the user space caller takes care of that (such as libkcapi)? By tinkering with
the SGLs and copying the data to the dst buffer before the cipher operation
takes place, I guess we will add performance degradation and more complexity
in the kernel.
Having such logic in user space would keep the algif_aead cleaner IMHO.
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management
From: Herbert Xu @ 2017-01-12 15:51 UTC (permalink / raw)
To: Stephan Müller; +Cc: linux-crypto
In-Reply-To: <2789022.qIBfP2h8Cy@positron.chronox.de>
On Sun, Dec 25, 2016 at 06:15:06PM +0100, Stephan Müller wrote:
>
> + * The following concept of the memory management is used:
> + *
> + * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is
> + * filled by user space with the data submitted via sendpage/sendmsg. Filling
> + * up the TX SGL does not cause a crypto operation -- the data will only be
> + * tracked by the kernel. Upon receipt of one recvmsg call, the caller must
> + * provide a buffer which is tracked with the RX SGL.
> + *
> + * During the processing of the recvmsg operation, the cipher request is
> + * allocated and prepared. To support multiple recvmsg operations operating
> + * on one TX SGL, an offset pointer into the TX SGL is maintained. The TX SGL
> + * that is used for the crypto request is scatterwalk_ffwd by the offset
> + * pointer to obtain the start address the crypto operation shall use for
> + * the input data.
I think this is overcomplicating things. The async interface
should be really simple. It should be exactly the same as the
sync interface, except that completion is out-of-line.
So there should be no mixing of SGLs from different requests.
Just start with a clean slate after each recvmsg regardless of
whether it's sync or async.
The only difference in the async case is that you need to keep a
reference to the old pages and free them upon completion. But this
should in no way interfere with subsequent requests.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management
From: Stephan Müller @ 2017-01-12 15:56 UTC (permalink / raw)
To: Herbert Xu; +Cc: linux-crypto
In-Reply-To: <20170112155127.GA19252@gondor.apana.org.au>
Am Donnerstag, 12. Januar 2017, 23:51:28 CET schrieb Herbert Xu:
Hi Herbert,
> On Sun, Dec 25, 2016 at 06:15:06PM +0100, Stephan Müller wrote:
> > + * The following concept of the memory management is used:
> > + *
> > + * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL
> > is
> > + * filled by user space with the data submitted via sendpage/sendmsg.
> > Filling + * up the TX SGL does not cause a crypto operation -- the data
> > will only be + * tracked by the kernel. Upon receipt of one recvmsg call,
> > the caller must + * provide a buffer which is tracked with the RX SGL.
> > + *
> > + * During the processing of the recvmsg operation, the cipher request is
> > + * allocated and prepared. To support multiple recvmsg operations
> > operating + * on one TX SGL, an offset pointer into the TX SGL is
> > maintained. The TX SGL + * that is used for the crypto request is
> > scatterwalk_ffwd by the offset + * pointer to obtain the start address
> > the crypto operation shall use for + * the input data.
>
> I think this is overcomplicating things. The async interface
> should be really simple. It should be exactly the same as the
> sync interface, except that completion is out-of-line.
>
> So there should be no mixing of SGLs from different requests.
> Just start with a clean slate after each recvmsg regardless of
> whether it's sync or async.
>
> The only difference in the async case is that you need to keep a
> reference to the old pages and free them upon completion. But this
> should in no way interfere with subsequent requests.
That would mean that we would only support one IOCB.
At least with algif_skcipher, having multiple IOCBs would reduce the number of
system calls user space needs to make for multiple plaintext / ciphertext
blocks. But then, with the use of IOVECs, user space could provide all input
data with one system call anyway.
Ok, I will update the patch as suggested.
>
> Cheers,
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
From: Stephan Müller @ 2017-01-12 16:01 UTC (permalink / raw)
To: Herbert Xu; +Cc: linux-crypto
In-Reply-To: <20170112155344.GB19252@gondor.apana.org.au>
Am Donnerstag, 12. Januar 2017, 23:53:44 CET schrieb Herbert Xu:
Hi Herbert,
>
> > If we only want to solve that for algif_aead, wouldn't it make more sense
> > if the user space caller takes care of that (such as libkcapi)? By
> > tinkering with the SGLs and copying the data to the dst buffer before the
> > cipher operation takes place, I guess we will add performance degradation
> > and more complexity in the kernel.
> >
> > Having such logic in user space would keep the algif_aead cleaner IMHO.
>
> We need to have a sane kernel API that respects POSIX.
I fully agree. Therefore, I was under the impression that disregarding the AAD
in recvmsg entirely would be most appropriate as offered with the patch
"crypto: AF_ALG - disregard AAD buffer space for output". In this case we
would be fully POSIX compliant, the kernel would not copy the AAD (and thus
perform multiple memcpy operations due to copy_from_user and copy_to_user
round trips) and leave the AAD copy operation entirely to user space.
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management
From: Stephan Müller @ 2017-01-12 16:05 UTC (permalink / raw)
To: Herbert Xu; +Cc: linux-crypto
In-Reply-To: <3051008.MbfH4VO98W@tauon.atsec.com>
Am Donnerstag, 12. Januar 2017, 16:56:04 CET schrieb Stephan Müller:
Hi Herbert,
>
> That would mean that we would only support one IOCB.
As we also need to be standards compliant, would it be appropriate to only
support one IOCB? I think this is a significant difference to other AIO
operations like for networking.
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
From: Herbert Xu @ 2017-01-12 16:06 UTC (permalink / raw)
To: Stephan Müller; +Cc: linux-crypto
In-Reply-To: <1604413.m50iU9qQPD@tauon.atsec.com>
On Thu, Jan 12, 2017 at 05:01:13PM +0100, Stephan Müller wrote:
>
> I fully agree. Therefore, I was under the impression that disregarding the AAD
> in recvmsg entirely would be most appropriate as offered with the patch
> "crypto: AF_ALG - disregard AAD buffer space for output". In this case we
> would be fully POSIX compliant, the kernel would not copy the AAD (and thus
> perform multiple memcpy operations due to copy_from_user and copy_to_user
> round trips) and leave the AAD copy operation entirely to user space.
Yes but then you'd have to play nasty games to fit this through
the kernel API. Besides, we could still do in-place crypto even
though you suggested that it's complicated. It's not really.
All we have to do is walk through the SG list and compare each
page/offset. For the common case it's going to be a single-entry
list.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management
From: Herbert Xu @ 2017-01-12 16:07 UTC (permalink / raw)
To: Stephan Müller; +Cc: linux-crypto
In-Reply-To: <2701907.5SyJthQSdc@tauon.atsec.com>
On Thu, Jan 12, 2017 at 05:05:03PM +0100, Stephan Müller wrote:
> Am Donnerstag, 12. Januar 2017, 16:56:04 CET schrieb Stephan Müller:
>
> Hi Herbert,
>
> >
> > That would mean that we would only support one IOCB.
>
> As we also need to be standards compliant, would it be appropriate to only
> support one IOCB? I think this is a significant difference to other AIO
> operations like for networking.
Why would we be limited to one IOCB?
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v1 3/8] crypto:chcr- Fix key length for RFC4106
From: Herbert Xu @ 2017-01-12 16:09 UTC (permalink / raw)
To: Harsh Jain; +Cc: hariprasad, netdev, linux-crypto
In-Reply-To: <6d8e61299e051d51dacdb6bfd6c5e582b230027c.1483599449.git.harsh@chelsio.com>
On Fri, Jan 06, 2017 at 02:01:34PM +0530, Harsh Jain wrote:
> Check keylen before copying salt to avoid wrap around of Integer.
>
> Signed-off-by: Harsh Jain <harsh@chelsio.com>
> ---
> drivers/crypto/chelsio/chcr_algo.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
> index deec7c0..6c2dea3 100644
> --- a/drivers/crypto/chelsio/chcr_algo.c
> +++ b/drivers/crypto/chelsio/chcr_algo.c
> @@ -2194,8 +2194,8 @@ static int chcr_gcm_setkey(struct crypto_aead *aead, const u8 *key,
> unsigned int ck_size;
> int ret = 0, key_ctx_size = 0;
>
> - if (get_aead_subtype(aead) ==
> - CRYPTO_ALG_SUB_TYPE_AEAD_RFC4106) {
> + if (get_aead_subtype(aead) == CRYPTO_ALG_SUB_TYPE_AEAD_RFC4106 &&
> + keylen > 3) {
> keylen -= 4; /* nonce/salt is present in the last 4 bytes */
> memcpy(aeadctx->salt, key + keylen, 4);
> }
We should return an error in this case.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 1/2] crypto: aead AF_ALG - overhaul memory management
From: Stephan Müller @ 2017-01-12 16:10 UTC (permalink / raw)
To: Herbert Xu; +Cc: linux-crypto
In-Reply-To: <20170112160739.GB19577@gondor.apana.org.au>
Am Freitag, 13. Januar 2017, 00:07:39 CET schrieb Herbert Xu:
Hi Herbert,
> On Thu, Jan 12, 2017 at 05:05:03PM +0100, Stephan Müller wrote:
> > Am Donnerstag, 12. Januar 2017, 16:56:04 CET schrieb Stephan Müller:
> >
> > Hi Herbert,
> >
> > > That would mean that we would only support one IOCB.
> >
> > As we also need to be standards compliant, would it be appropriate to only
> > support one IOCB? I think this is a significant difference to other AIO
> > operations like for networking.
>
> Why would we be limited to one IOCB?
Each IOCB would transpire into an independent, separate recvmsg invocation
without an additional sendmsg/sendpage operation. Thus, in order to support
multiple IOCBs, all data the multiple recvmsg invocations will operate on must
be injected into the kernel beforehand.
Ciao
Stephan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox