Re: PBKDF2 support in the linux kernel

[Jeffrey Walton]

On Thu, May 24, 2018 at 5:11 AM, Stephan Mueller  wrote:
> Am Donnerstag, 24. Mai 2018, 10:33:07 CEST schrieb Rafael J. Wysocki:
>
> Hi Rafael,
>
>> So the problem is that Yu would like to use this for hibernation encryption
>> done entirely in the kernel.
>
> But why do you need to perform PBKDF in kernel space?

I may be mis-parsing things, but using audited kernel code is a matter
of governance and good security engineering. I don't believe it is not
a matter of laziness.

If he/she were to add their own userland code then he would surely be
criticized for rolling his own implementation.

Jeff

Re: WARNING: kernel stack regs has bad 'bp' value (3)

[Eric Biggers]

On Sat, May 12, 2018 at 10:43:08AM +0200, Dmitry Vyukov wrote:
> On Fri, Feb 2, 2018 at 11:18 PM, Eric Biggers  wrote:
> > On Fri, Feb 02, 2018 at 02:57:32PM +0100, Dmitry Vyukov wrote:
> >> On Fri, Feb 2, 2018 at 2:48 PM, syzbot
> >>  wrote:
> >> > Hello,
> >> >
> >> > syzbot hit the following crash on upstream commit
> >> > 7109a04eae81c41ed529da9f3c48c3655ccea741 (Thu Feb 1 17:37:30 2018 +)
> >> > Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide
> >> >
> >> > So far this crash happened 4 times on net-next, upstream.
> >> > C reproducer is attached.
> >> > syzkaller reproducer is attached.
> >> > Raw console output is attached.
> >> > compiler: gcc (GCC) 7.1.1 20170620
> >> > .config is attached.
> >>
> >>
> >> From suspicious frames I see salsa20_asm_crypt there, so +crypto 
> >> maintainers.
> >>
> >
> > Looks like the x86 implementations of Salsa20 (both i586 and x86_64) need 
> > to be
> > updated to not use %ebp/%rbp.
> 
> Ard,
> 
> This was bisected as introduced by:
> 
> commit 83dee2ce1ae791c3dc0c9d4d3a8d42cb109613f6
> Author: Ard Biesheuvel 
> Date:   Fri Jan 19 12:04:34 2018 +
> 
> crypto: sha3-generic - rewrite KECCAK transform to help the
> compiler optimize
> 
> https://gist.githubusercontent.com/dvyukov/47f93f5a0679170dddf93bc019b42f6d/raw/65beac8ddd30003bbd4e9729236dc8572094abf7/gistfile1.txt

Note that syzbot's original C reproducer (from Feb 1) for this actually
triggered the warning through salsa20-asm, which I've just proposed to "fix" by
https://patchwork.kernel.org/patch/10428863/.  sha3-generic is apparently
another instance of the same bug, where the %rbp register is used for data.

Eric

Re: PBKDF2 support in the linux kernel

[Herbert Xu]

On Tue, May 22, 2018 at 11:00:40AM +0800, Yu Chen wrote:
> Hi all,
> The request is that, we'd like to generate a symmetric key derived from
> user provided passphase(not rely on any third-party library). May I know if
> there is a PBKDF2(Password-Based Key Derivation Function 2) support in the
> kernel? (https://tools.ietf.org/html/rfc2898#5.2)
> We have hmac sha1 in the kernel, do we have plan to port/implement
> corresponding PBKDF2 in the kernel too?

The rule for adding crypto code to the kernel is simple, there
must be an in-kernel user of the algorithm.

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

Re: PBKDF2 support in the linux kernel

[Eric Biggers]

Hi Denis,

On Fri, May 25, 2018 at 09:48:36AM -0500, Denis Kenzior wrote:
> Hi Eric,
> 
> > The solution to the "too many system calls" problem is trivial: just do 
> > SHA-512
> > in userspace.  It's just math; you don't need a system call, any more than 
> > you
> > would call sys_add(1, 1) to compute 1 + 1.  The CPU instructions that can
> > accelerate SHA-512, such as AVX and ARM CE, are available in userspace too; 
> > and
> > there are tons of libraries already available that implement it for you.  
> > Your
> > argument isn't fundamentally different from saying that sys_leftpad() (if 
> > we had
> > the extraordinary misfortune of it actually existing) is too slow, so we 
> > should
> > add a Javascript interpreter to the kernel.
> 
> So lets recap.  The Kernel crypto framework is something that:
> a) (some, many?) people are totally happy with, it does everything that they
> want
> b) is peer reviewed by the best programmers in the world
> c) responds / fixes vulnerabilities almost instantly
> d) automatically picks the best software optimized version of a given crypto
> algorithm for us
> e) automagically uses hardware optimization if the system supports it
> f) API compatibility is essentially guaranteed forever
> g) Maybe not the most performant in the world, but to many users this
> doesn't matter.
> 
> So your response to those users is to please stop using what works well and
> start adding random crypto code from the internet into their project?
> Something that likely won't do a, b, c, d, e or f above just because *oh
> gosh* we might find and have to fix some bugs in the kernel?  Have you
> actually thought through how that sounds?
> 
> What you call laziness I call 'common sense' and 'good security practice.'
> Does using the kernel make sense for everyone? No.  But for some it does.
> So if there's a legitimate way to make things better, can we not discuss
> them civilly?

I've explained this already -- it is exactly the *opposite* of good security
practice to increase the attack surface of ring 0 code.  Have *you* actually
thought about how it ridiculous it is elevate privileges just to do math?  We
need to be reducing the kernel attack surface, not increasing it.

It's great that you're confident in me, Herbert, Stephen, and other people who
contribute to the Linux crypto API.  The reality though is that there are still
known denial of service vulnerabilities that no one has found time to fix yet,
like deadlocking the kernel through recursive pcrypt, or exhausting kernel
memory by instantiating an unbounded number of crypto algorithms.  These types
of bugs aren't relevant for userspace crypto libraries as it would just be an
application shooting itself in the foot, but in the kernel they are a problem
for everyone, at least without e.g. an SELinux policy in place to lock down the
attack surface.

Again, all your arguments minus the crypto-specific parts also apply to
sys_leftpad(), which to be very clear was an April Fool's joke, and not an
actual proposal.  I trust that this wasn't meant to be a very late April Fool's
joke too :-)

> 
> > 
> > Also note that in the rare cases where the kernel actually does do very long
> > running calculations for whatever reason, kernel developers pretty regularly
> > screw it up by forgetting to insert explicit preemption points 
> > (cond_resched()),
> > or (slightly less bad) making it noninterruptible.  I had to "fix" one of 
> > these,
> > where someone for whatever reason added a keyctl() operation that does
> > Diffie-Hellman key exchange in software.  In !CONFIG_PREEMPT kernels any
> > unprivileged user could call it to lock up all CPUs for 20+ seconds, meaning
> > that no other processes can be scheduled on them.  This isn't a problem at 
> > all
> > in userspace.
> 
> And this is exactly why people should _want_ to use the kernel crypto
> framework.  Because people like you exist and fix such issues.  So again,
> kudos :)
> 

No, it's exactly why people should *not* want to do crypto in the kernel,
because that class of bug cannot exist in userspace code.

Eric

Re: PBKDF2 support in the linux kernel

[Denis Kenzior]

Hi Eric,


The solution to the "too many system calls" problem is trivial: just do SHA-512
in userspace.  It's just math; you don't need a system call, any more than you
would call sys_add(1, 1) to compute 1 + 1.  The CPU instructions that can
accelerate SHA-512, such as AVX and ARM CE, are available in userspace too; and
there are tons of libraries already available that implement it for you.  Your
argument isn't fundamentally different from saying that sys_leftpad() (if we had
the extraordinary misfortune of it actually existing) is too slow, so we should
add a Javascript interpreter to the kernel.


So lets recap.  The Kernel crypto framework is something that:
a) (some, many?) people are totally happy with, it does everything that 
they want

b) is peer reviewed by the best programmers in the world
c) responds / fixes vulnerabilities almost instantly
d) automatically picks the best software optimized version of a given 
crypto algorithm for us

e) automagically uses hardware optimization if the system supports it
f) API compatibility is essentially guaranteed forever
g) Maybe not the most performant in the world, but to many users this 
doesn't matter.


So your response to those users is to please stop using what works well 
and start adding random crypto code from the internet into their 
project?  Something that likely won't do a, b, c, d, e or f above just 
because *oh gosh* we might find and have to fix some bugs in the kernel? 
 Have you actually thought through how that sounds?


What you call laziness I call 'common sense' and 'good security 
practice.'  Does using the kernel make sense for everyone? No.  But for 
some it does.  So if there's a legitimate way to make things better, can 
we not discuss them civilly?




Also note that in the rare cases where the kernel actually does do very long
running calculations for whatever reason, kernel developers pretty regularly
screw it up by forgetting to insert explicit preemption points (cond_resched()),
or (slightly less bad) making it noninterruptible.  I had to "fix" one of these,
where someone for whatever reason added a keyctl() operation that does
Diffie-Hellman key exchange in software.  In !CONFIG_PREEMPT kernels any
unprivileged user could call it to lock up all CPUs for 20+ seconds, meaning
that no other processes can be scheduled on them.  This isn't a problem at all
in userspace.


And this is exactly why people should _want_ to use the kernel crypto 
framework.  Because people like you exist and fix such issues.  So 
again, kudos :)


Regards,
-Denis

Re: PBKDF2 support in the linux kernel

[Theodore Y. Ts'o]

On Fri, May 25, 2018 at 12:07:06PM +0200, Tomas Mraz wrote:
> 
> Because having millions of copies of SHA1, MD5, and SHA2 and  in
> millions of applications is the best thing.
>
> Now that's something I would call laziness - just copy the code and do
> not care about doing the proper decision which crypto library to use.

These algorithms are static and have test vectors.  If you don't need
hardware acceleration for your use case, and portability and reducing
external dependencies are a priority, it's a very realistic
engineering tradeoff.

libext2fs has been ABI backwards compatible for 19 years (since the
move from a.out to ELF shared libraries).  OpenSSL can't keep ABI
compatibility from one relase to another.  You can't build ABI
compatibility on top of shifting sands, so that's a really good reason
for a library not to depend on OpenSSL (if you care about backwards
compatibility, anyway).

Also consider that sha512.o is only 4735 bytes.  libxml2 has a size of
1.75 megabytes, so having my own version of sha512 is equivalent to
0.26% of libxml2.

Using my own copy of sha512?  2.5 milli-libxml2's.  Shared library ABI
backwards compatibility?  Priceless.

(And I won't even get into the bloat-o-rama which is GNOME2)

   - Ted

Re: PBKDF2 support in the linux kernel

[Tomas Mraz]

On Thu, 2018-05-24 at 20:42 -0400, Theodore Y. Ts'o wrote:
> On Thu, May 24, 2018 at 07:09:27PM -0500, Denis Kenzior wrote:
> > 
> > But seriously, how is it a fault of the 'random person on the
> > mailing list'
> > that AF_ALG exists and is being used for its (seemingly intended)
> > purpose?
> > 
> > I'm not really here to criticize or judge the past.  AF_ALG exists
> > now. It
> > is being used.  Can we just make it better?  Or are we going to
> > whinge at
> > every user that tries to use (and improve) kernel features that
> > (some)
> > people disagree with because it can 'compromise' kernel security?
> 
> Another point of view is that it was arguably a mistake, and we
> shouldn't make it worse.
> 
> > > Also, if speed isn't a worry, why not just a single software-only
> > > implementation of SHA1, and be done with it?  It's what I did in
> > > e2fsprogs for e4crypt.
> > 
> > If things were that simple, we would definitely not be having this
> > exchange.
> > Lets just say we use just about every feature that crypto subsystem
> > provides
> > in some way.
> 
> What I'm saying here is if you need to code PBPDF2 in user-space, it
> _really_ isn't hard.  I've done it.  It's less than 7k of source code
> to implement SHA512:
> 
> https://ghit.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/lib/ext2fs
> /sha256.c
> 
> and then less than 50 lines of code to implement PBPDF2 (I didn't
> even
> bother putting in a library such as libext2fs):
> 
> https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/misc/e4cryp
> t.c#n405
> 
> This is all you would need to do if we don't put PBPDF2 in the
> kernel.
> Is it really that onerous?
> 
> If you don't want to use some crypto library, I don't blame you --- I
> just grabbed the code and dropped it into e2fsprogs; so I understand
> that POV.  And so if you want to keep using AF_ALG, we made a
> mistake,
> created an attractive nuisance, and we have to support it forever.
> Fine.  But we don't have to add more _stuff_ into the kernel

Because having millions of copies of SHA1, MD5, and SHA2 and  in
millions of applications is the best thing. 

Now that's something I would call laziness - just copy the code and do
not care about doing the proper decision which crypto library to use.

LOL

-- 
Tomáš Mráz
No matter how far down the wrong road you've gone, turn back.
  Turkish proverb
[You'll know whether the road is wrong if you carefully listen to your
conscience.]

Re: PBKDF2 support in the linux kernel

[Eric Biggers]

Hi Denis,

On Thu, May 24, 2018 at 07:56:50PM -0500, Denis Kenzior wrote:
> Hi Ted,
> 
> > > I'm not really here to criticize or judge the past.  AF_ALG exists now. It
> > > is being used.  Can we just make it better?  Or are we going to whinge at
> > > every user that tries to use (and improve) kernel features that (some)
> > > people disagree with because it can 'compromise' kernel security?
> > 
> > Another point of view is that it was arguably a mistake, and we
> > shouldn't make it worse.
> 
> Fair enough.  I'm just voicing the opposite point of view.  Namely that you
> have created something nice, and useful.  Even if it turned out not quite
> like you thought it would be in hindsight.
> 
> > 
> > > > Also, if speed isn't a worry, why not just a single software-only
> > > > implementation of SHA1, and be done with it?  It's what I did in
> > > > e2fsprogs for e4crypt.
> > > 
> > > If things were that simple, we would definitely not be having this 
> > > exchange.
> > > Lets just say we use just about every feature that crypto subsystem 
> > > provides
> > > in some way.
> > 
> > What I'm saying here is if you need to code PBPDF2 in user-space, it
> > _really_ isn't hard.  I've done it.  It's less than 7k of source code
> > to implement SHA512:
> > 
> > https://ghit.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/lib/ext2fs/sha256.c
> > 
> > and then less than 50 lines of code to implement PBPDF2 (I didn't even
> > bother putting in a library such as libext2fs):
> > 
> > https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/misc/e4crypt.c#n405
> > 
> > This is all you would need to do if we don't put PBPDF2 in the kernel.
> > Is it really that onerous?
> 
> No.  And in fact if you read upthread you will notice that I provided a link
> to our implementation of both PBKDF1 & 2 and they're as small as you say.
> They do everything we need.  So I'm right there with you.
> 
> But, PBKDF uses like 4K iterations (for WiFi passphrase -> key conversion
> for example) to arrive at its solution.  So you have implementations
> hammering the kernel with system calls.
> 
> So we can whinge at these implementations for 'being lazy', wring our hands,
> say how everything was just a big mistake.  Or maybe we can do something so
> that the kernel isn't hammered needlessly...
> 

The solution to the "too many system calls" problem is trivial: just do SHA-512
in userspace.  It's just math; you don't need a system call, any more than you
would call sys_add(1, 1) to compute 1 + 1.  The CPU instructions that can
accelerate SHA-512, such as AVX and ARM CE, are available in userspace too; and
there are tons of libraries already available that implement it for you.  Your
argument isn't fundamentally different from saying that sys_leftpad() (if we had
the extraordinary misfortune of it actually existing) is too slow, so we should
add a Javascript interpreter to the kernel.

Also note that in the rare cases where the kernel actually does do very long
running calculations for whatever reason, kernel developers pretty regularly
screw it up by forgetting to insert explicit preemption points (cond_resched()),
or (slightly less bad) making it noninterruptible.  I had to "fix" one of these,
where someone for whatever reason added a keyctl() operation that does
Diffie-Hellman key exchange in software.  In !CONFIG_PREEMPT kernels any
unprivileged user could call it to lock up all CPUs for 20+ seconds, meaning
that no other processes can be scheduled on them.  This isn't a problem at all
in userspace.

Eric

Re: PBKDF2 support in the linux kernel

[Denis Kenzior]

Hi Ted,


I'm not really here to criticize or judge the past.  AF_ALG exists now. It
is being used.  Can we just make it better?  Or are we going to whinge at
every user that tries to use (and improve) kernel features that (some)
people disagree with because it can 'compromise' kernel security?


Another point of view is that it was arguably a mistake, and we
shouldn't make it worse.


Fair enough.  I'm just voicing the opposite point of view.  Namely that 
you have created something nice, and useful.  Even if it turned out not 
quite like you thought it would be in hindsight.





Also, if speed isn't a worry, why not just a single software-only
implementation of SHA1, and be done with it?  It's what I did in
e2fsprogs for e4crypt.


If things were that simple, we would definitely not be having this exchange.
Lets just say we use just about every feature that crypto subsystem provides
in some way.


What I'm saying here is if you need to code PBPDF2 in user-space, it
_really_ isn't hard.  I've done it.  It's less than 7k of source code
to implement SHA512:

https://ghit.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/lib/ext2fs/sha256.c

and then less than 50 lines of code to implement PBPDF2 (I didn't even
bother putting in a library such as libext2fs):

https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/misc/e4crypt.c#n405

This is all you would need to do if we don't put PBPDF2 in the kernel.
Is it really that onerous?


No.  And in fact if you read upthread you will notice that I provided a 
link to our implementation of both PBKDF1 & 2 and they're as small as 
you say.  They do everything we need.  So I'm right there with you.


But, PBKDF uses like 4K iterations (for WiFi passphrase -> key 
conversion for example) to arrive at its solution.  So you have 
implementations hammering the kernel with system calls.


So we can whinge at these implementations for 'being lazy', wring our 
hands, say how everything was just a big mistake.  Or maybe we can do 
something so that the kernel isn't hammered needlessly...


Regards,
-Denis

Re: PBKDF2 support in the linux kernel

[Theodore Y. Ts'o]

One more thought about why userspace using AF_ALG is a bad idea ---
there is no guarantee that all kernels will have all of the crypto
algorithms you need built into the kernel.  People who build custom
kernels very often only include those kernel modules they need.  So by
default I don't normally build the more exotic crypto algorithms into
my kernel --- and some people might not the crypto algorithms _you_
care about built into the kernel.  (Not every one uses distro
kernels.)

So if you want your program to work everywhere, you're going to have
to provide fallback crypto algorithms anyway.  Which is why arguably
it was a Really Bad Idea that AF_ALG provides access to software-only
crypto implementations in the kernel.  It led userspace programmers
down the primrose path into making programs that are fragile with
respect to users with custom-built kernels.

- Ted

Re: PBKDF2 support in the linux kernel

[Theodore Y. Ts'o]

On Thu, May 24, 2018 at 07:09:27PM -0500, Denis Kenzior wrote:
> 
> But seriously, how is it a fault of the 'random person on the mailing list'
> that AF_ALG exists and is being used for its (seemingly intended) purpose?
> 
> I'm not really here to criticize or judge the past.  AF_ALG exists now. It
> is being used.  Can we just make it better?  Or are we going to whinge at
> every user that tries to use (and improve) kernel features that (some)
> people disagree with because it can 'compromise' kernel security?

Another point of view is that it was arguably a mistake, and we
shouldn't make it worse.

> > Also, if speed isn't a worry, why not just a single software-only
> > implementation of SHA1, and be done with it?  It's what I did in
> > e2fsprogs for e4crypt.
> 
> If things were that simple, we would definitely not be having this exchange.
> Lets just say we use just about every feature that crypto subsystem provides
> in some way.

What I'm saying here is if you need to code PBPDF2 in user-space, it
_really_ isn't hard.  I've done it.  It's less than 7k of source code
to implement SHA512:

https://ghit.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/lib/ext2fs/sha256.c

and then less than 50 lines of code to implement PBPDF2 (I didn't even
bother putting in a library such as libext2fs):

https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/misc/e4crypt.c#n405

This is all you would need to do if we don't put PBPDF2 in the kernel.
Is it really that onerous?

If you don't want to use some crypto library, I don't blame you --- I
just grabbed the code and dropped it into e2fsprogs; so I understand
that POV.  And so if you want to keep using AF_ALG, we made a mistake,
created an attractive nuisance, and we have to support it forever.
Fine.  But we don't have to add more _stuff_ into the kernel

- Ted

Re: PBKDF2 support in the linux kernel

[Denis Kenzior]

Hi Ted,

On 05/24/2018 06:25 PM, Theodore Y. Ts'o wrote:

On Thu, May 24, 2018 at 05:08:41PM -0500, Denis Kenzior wrote:

Actually for the use case we have, speed is something pretty low on the
priority list; not having to deal with userspace crypto library dependencies
was a goal in and of itself.  Each one has its own issues and you can never
support just one.  Using AF_ALG has been rather... liberating.


Which is probably why Eric used the word, "laziness".  You might use a
different word, but the decisoin was one that was more driven by
convenience than kernel security


Err, this is a bit uncalled for.

But seriously, how is it a fault of the 'random person on the mailing 
list' that AF_ALG exists and is being used for its (seemingly intended) 
purpose?


I'm not really here to criticize or judge the past.  AF_ALG exists now. 
It is being used.  Can we just make it better?  Or are we going to 
whinge at every user that tries to use (and improve) kernel features 
that (some) people disagree with because it can 'compromise' kernel 
security?




Also, if speed isn't a worry, why not just a single software-only
implementation of SHA1, and be done with it?  It's what I did in
e2fsprogs for e4crypt.


If things were that simple, we would definitely not be having this 
exchange.  Lets just say we use just about every feature that crypto 
subsystem provides in some way.


Regards,
-Denis

Re: PBKDF2 support in the linux kernel

[Theodore Y. Ts'o]

On Thu, May 24, 2018 at 05:08:41PM -0500, Denis Kenzior wrote:
> Actually for the use case we have, speed is something pretty low on the
> priority list; not having to deal with userspace crypto library dependencies
> was a goal in and of itself.  Each one has its own issues and you can never
> support just one.  Using AF_ALG has been rather... liberating.

Which is probably why Eric used the word, "laziness".  You might use a
different word, but the decisoin was one that was more driven by
convenience than kernel security

Also, if speed isn't a worry, why not just a single software-only
implementation of SHA1, and be done with it?  It's what I did in
e2fsprogs for e4crypt.

- Ted

Re: PBKDF2 support in the linux kernel

[Denis Kenzior]

Hi Ted,

On 05/24/2018 04:05 PM, Theodore Y. Ts'o wrote:

Has anyone actually done the experiment and verified that it was in
fact a win to use AF_ALG on at least _some_ platform?  What about the
common cast for most platforms, even those that had some kind of
hardware accleration that could only be accessed by the kernel?

Actually for the use case we have, speed is something pretty low on the 
priority list; not having to deal with userspace crypto library 
dependencies was a goal in and of itself.  Each one has its own issues 
and you can never support just one.  Using AF_ALG has been rather... 
liberating.


Regards,
-Denis

Re: PBKDF2 support in the linux kernel

[Theodore Y. Ts'o]

On Thu, May 24, 2018 at 12:11:35PM -0500, Denis Kenzior wrote:
> 
> Well, I'm not sure where the laziness comment is coming from.  We have at
> least two user-space implementations that implement PBKDF on top of AF_ALG.
> But a typical invocation of PBKDF runs a couple of thousand iterations.
> That is a lot of system call overhead.  Would it not be better to fix things
> to be more efficient rather than worry about how 'mistakes were made'?

Even where there is hardware acceleration, I suspect that it might be
more efficient (as in, result in a faster implementation) if the user
PBKDF application was changed to use its own in-userspace software
implementation.  Many/most hardware implementations are optimzied for
throughput (e.g., bulk data operations), and it's not obvious to me
that once you had the syscall overhead, it's actually faster to use
the hardware accleration.

Has anyone actually done the experiment and verified that it was in
fact a win to use AF_ALG on at least _some_ platform?  What about the
common cast for most platforms, even those that had some kind of
hardware accleration that could only be accessed by the kernel?

(Amusing war story: the hardware where we first experimented with ext4
encryption, the hardware "acceleration" offered by the ARM core in
question was *slower* than a well-tuned software-only implementation
on the same ARM CPU!  :-)

- Ted

Re: PBKDF2 support in the linux kernel

[Denis Kenzior]

Hi Eric,


No, we don't add random code to the kernel just because people are lazy.  IMO it
was a mistake that AF_ALG allows access to software crypto implementations by
default (as opposed to just hardware crypto devices), but it's not an excuse to


I understand, but my point is, AF_ALG is out there now and hand-wringing 
about how it is a bad idea is sort of late / misplaced...



add random other stuff to the kernel.  The kernel runs in a privileged context
under special constraints, e.g. non-preemptible in some configurations, and any
bug can crash or lock up the system, leak data, or even allow elevation of
privilege.  We're already dealing with hundreds of bugs in the kernel found by
fuzzing [1], many of which no one feels very responsible for fixing.  In fact


I can only speak for myself here, but yes, this is understood.  And I 
still disagree with your assessment.



about 20 bugs were reported in AF_ALG as soon as definitions for AF_ALG were
added to syzkaller; at least a couple were very likely exploitable to gain


And I saw your contributions fly by to fix a lot of these issues.  Given 
that our project depends on and heavily uses AF_ALG, this is really much 
appreciated!



arbitrary kernel code execution.  The last thing we need is adding even more
code to the kernel just because people are too lazy to write userspace code.  Do
we need sys_leftpad() [2] next?


Well, I'm not sure where the laziness comment is coming from.  We have 
at least two user-space implementations that implement PBKDF on top of 
AF_ALG.  But a typical invocation of PBKDF runs a couple of thousand 
iterations.  That is a lot of system call overhead.  Would it not be 
better to fix things to be more efficient rather than worry about how 
'mistakes were made'?


And yes, people can always try to take things too far (as you point out 
in [2]), but I would argue this isn't such a bad one.


Regards,
-Denis

Re: PBKDF2 support in the linux kernel

[Eric Biggers]

On Thu, May 24, 2018 at 09:36:15AM -0500, Denis Kenzior wrote:
> Hi Stephan,
> 
> On 05/24/2018 12:57 AM, Stephan Mueller wrote:
> > Am Donnerstag, 24. Mai 2018, 04:45:00 CEST schrieb Eric Biggers:
> > 
> > Hi Eric,
> > 
> > > 
> > > "Not having to rely on any third-party library" is not an excuse to add
> > > random code to the kernel, which runs in a privileged context.  Please do
> > > PBKDF2 in userspace instead.
> > 
> > I second that. Besides, if you really need to rely on the kernel crypto API 
> > to
> > do that because you do not want to add yet another crypto lib, libkcapi has 
> > a
> > PBKDF2 implementation that uses the kernel crypto API via AF_ALG. I.e. the
> > kernel crypto API is used and yet the PBKDF logic is in user space.
> > 
> > http://www.chronox.de/libkcapi.html
> > 
> 
> I actually don't see why we _shouldn't_ have PBKDF in the kernel.  We
> already have at least 2 user space libraries that implement it via AF_ALG.
> ell does this as well:
> https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/pkcs5.c
> 
> One can argue whether this is a good or bad idea, but the cat is out of the
> bag.
> 
> So from a practical perspective, would it not be better to make this an
> explicit kernel API and not have userspace hammer AF_ALG socket a few
> thousand times to do what it wants?
> 

No, we don't add random code to the kernel just because people are lazy.  IMO it
was a mistake that AF_ALG allows access to software crypto implementations by
default (as opposed to just hardware crypto devices), but it's not an excuse to
add random other stuff to the kernel.  The kernel runs in a privileged context
under special constraints, e.g. non-preemptible in some configurations, and any
bug can crash or lock up the system, leak data, or even allow elevation of
privilege.  We're already dealing with hundreds of bugs in the kernel found by
fuzzing [1], many of which no one feels very responsible for fixing.  In fact
about 20 bugs were reported in AF_ALG as soon as definitions for AF_ALG were
added to syzkaller; at least a couple were very likely exploitable to gain
arbitrary kernel code execution.  The last thing we need is adding even more
code to the kernel just because people are too lazy to write userspace code.  Do
we need sys_leftpad() [2] next?

[1] https://syzkaller.appspot.com/
[2] https://lkml.org/lkml/2016/3/31/1108

- Eric

Re: PBKDF2 support in the linux kernel

[Denis Kenzior]

Hi Stephan,

On 05/24/2018 12:57 AM, Stephan Mueller wrote:

Am Donnerstag, 24. Mai 2018, 04:45:00 CEST schrieb Eric Biggers:

Hi Eric,



"Not having to rely on any third-party library" is not an excuse to add
random code to the kernel, which runs in a privileged context.  Please do
PBKDF2 in userspace instead.


I second that. Besides, if you really need to rely on the kernel crypto API to
do that because you do not want to add yet another crypto lib, libkcapi has a
PBKDF2 implementation that uses the kernel crypto API via AF_ALG. I.e. the
kernel crypto API is used and yet the PBKDF logic is in user space.

http://www.chronox.de/libkcapi.html



I actually don't see why we _shouldn't_ have PBKDF in the kernel.  We 
already have at least 2 user space libraries that implement it via 
AF_ALG.  ell does this as well:

https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/pkcs5.c

One can argue whether this is a good or bad idea, but the cat is out of 
the bag.


So from a practical perspective, would it not be better to make this an 
explicit kernel API and not have userspace hammer AF_ALG socket a few 
thousand times to do what it wants?


Regards,
-Denis

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 24, 2018 at 3:20 PM, Gilad Ben-Yossef  wrote:
> On Tue, May 22, 2018 at 10:48 AM, Geert Uytterhoeven
>  wrote:
>> On Mon, May 21, 2018 at 3:43 PM, Gilad Ben-Yossef  
>> wrote:
>>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>>>  wrote:
 Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
 does not distinguish between the absence of the clock property, and an
 actual error in getting the clock, and never considers any error a failure
 (incl. -PROBE_DEFER).

 As of_clk_get() returns -ENOENT for both a missing clock property and a
 missing clock, you should use (devm_)clk_get() instead, and distinguish
 between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>>>
>>> I was trying to do as you suggested but I didn't quite get what is the
>>> dev_id (2nd) parameter to devm_clk_get parameter is supposed to be.
>>
>> It's the (optional) name of the clock, helpful in case there is more than 
>> one.
>> In your case, NULL is fine.
>
> I have assumed as much and tried it, it did not work and so I assumed
> I am missing something and asked you.
> It turns out I was missing the fact I was using the wrong device tree
> file... :-(
>
> So thanks, it works now :-)

Glad to hear that!

> Having said that, while using devm)clk_get() is a better approach, it
> does not seems to distinguish
> between no "clocks" and a failure to clock information - it returns
> ENOENT in both cases as well.

Oh right, I guess I'm too used to not even getting that far due to the PM
Domain code failing to obtain the clock...

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Tue, May 22, 2018 at 10:48 AM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> On Mon, May 21, 2018 at 3:43 PM, Gilad Ben-Yossef  wrote:
>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>>  wrote:
>>> Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
>>> does not distinguish between the absence of the clock property, and an
>>> actual error in getting the clock, and never considers any error a failure
>>> (incl. -PROBE_DEFER).
>>>
>>> As of_clk_get() returns -ENOENT for both a missing clock property and a
>>> missing clock, you should use (devm_)clk_get() instead, and distinguish
>>> between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>>
>> I was trying to do as you suggested but I didn't quite get what is the
>> dev_id (2nd) parameter to devm_clk_get parameter is supposed to be.
>
> It's the (optional) name of the clock, helpful in case there is more than one.
> In your case, NULL is fine.
>

I have assumed as much and tried it, it did not work and so I assumed
I am missing something and asked you.
It turns out I was missing the fact I was using the wrong device tree
file... :-(

So thanks, it works now :-)

Having said that, while using devm)clk_get() is a better approach, it
does not seems to distinguish
between no "clocks" and a failure to clock information - it returns
ENOENT in both cases as well.

Thanks,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: PBKDF2 support in the linux kernel

[Rafael J. Wysocki]

On Thursday, May 24, 2018 11:36:04 AM CEST Stephan Mueller wrote:
> Am Donnerstag, 24. Mai 2018, 11:27:56 CEST schrieb Rafael J. Wysocki:
> 
> Hi Rafael,
> 
> > On Thursday, May 24, 2018 11:11:32 AM CEST Stephan Mueller wrote:
> > > Am Donnerstag, 24. Mai 2018, 10:33:07 CEST schrieb Rafael J. Wysocki:
> > > 
> > > Hi Rafael,
> > 
> > Hi Stephan,
> > 
> > > > So the problem is that Yu would like to use this for hibernation
> > > > encryption
> > > > done entirely in the kernel.
> > > 
> > > But why do you need to perform PBKDF in kernel space?
> > > 
> > > If you retain the password information in the kernel, you could retain the
> > > derived key instead of the passcode.
> > > 
> > > If, however, you ask for the user password during resume, you need some
> > > user space component to query that password. The PBKDF can also be
> > > handled in user space along with the query.
> > 
> > In principle it is possible to pass a key to the kernel from user
> > space, but that doesn't guarantee the key to be a really good one.  It
> > depends on a user space component to do the right thing and IMO that is
> > risky.
> > 
> > And please note that the information contained in hibernation images may
> > be extremely sensitive (keys and similar).
> > 
> > > Or how do you want to handle the passcode?
> > 
> > The idea is to write a passphrase to the kernel via something like sysfs,
> > generate a key from it on the fly and store the key.
> 
> But what is the difference between a passcode and the key derived from it. 
> The 
> derived key should only spread the assumed entropy in the passcode evenly. In 
> addition with its iteration count, it shall ensure that offline brute force 
> attacks of the passcode using stored protected data is made hard.
> 
> So, I do not see why it is risky to do the PBKDF in user space and then hand 
> the key to the kernel at runtime. It does not really matter if the attacker 
> sees the plaintext key or the passcode. If the attacker can access the kernel/
> user communication channel, we have a problem in any case.

OK, we'll follow this advice, thanks!

Best,
Rafael

Re: PBKDF2 support in the linux kernel

[Stephan Mueller]

Am Donnerstag, 24. Mai 2018, 11:27:56 CEST schrieb Rafael J. Wysocki:

Hi Rafael,

> On Thursday, May 24, 2018 11:11:32 AM CEST Stephan Mueller wrote:
> > Am Donnerstag, 24. Mai 2018, 10:33:07 CEST schrieb Rafael J. Wysocki:
> > 
> > Hi Rafael,
> 
> Hi Stephan,
> 
> > > So the problem is that Yu would like to use this for hibernation
> > > encryption
> > > done entirely in the kernel.
> > 
> > But why do you need to perform PBKDF in kernel space?
> > 
> > If you retain the password information in the kernel, you could retain the
> > derived key instead of the passcode.
> > 
> > If, however, you ask for the user password during resume, you need some
> > user space component to query that password. The PBKDF can also be
> > handled in user space along with the query.
> 
> In principle it is possible to pass a key to the kernel from user
> space, but that doesn't guarantee the key to be a really good one.  It
> depends on a user space component to do the right thing and IMO that is
> risky.
> 
> And please note that the information contained in hibernation images may
> be extremely sensitive (keys and similar).
> 
> > Or how do you want to handle the passcode?
> 
> The idea is to write a passphrase to the kernel via something like sysfs,
> generate a key from it on the fly and store the key.

But what is the difference between a passcode and the key derived from it. The 
derived key should only spread the assumed entropy in the passcode evenly. In 
addition with its iteration count, it shall ensure that offline brute force 
attacks of the passcode using stored protected data is made hard.

So, I do not see why it is risky to do the PBKDF in user space and then hand 
the key to the kernel at runtime. It does not really matter if the attacker 
sees the plaintext key or the passcode. If the attacker can access the kernel/
user communication channel, we have a problem in any case.

Or do I miss something here?

Ciao
Stephan

Re: PBKDF2 support in the linux kernel

[Rafael J. Wysocki]

On Thursday, May 24, 2018 11:11:32 AM CEST Stephan Mueller wrote:
> Am Donnerstag, 24. Mai 2018, 10:33:07 CEST schrieb Rafael J. Wysocki:
> 
> Hi Rafael,

Hi Stephan,

> > So the problem is that Yu would like to use this for hibernation encryption
> > done entirely in the kernel.
> 
> But why do you need to perform PBKDF in kernel space?
> 
> If you retain the password information in the kernel, you could retain the 
> derived key instead of the passcode.
> 
> If, however, you ask for the user password during resume, you need some user 
> space component to query that password. The PBKDF can also be handled in user 
> space along with the query.

In principle it is possible to pass a key to the kernel from user
space, but that doesn't guarantee the key to be a really good one.  It
depends on a user space component to do the right thing and IMO that is
risky.

And please note that the information contained in hibernation images may
be extremely sensitive (keys and similar).

> Or how do you want to handle the passcode?

The idea is to write a passphrase to the kernel via something like sysfs,
generate a key from it on the fly and store the key.

> > 
> > The exact use case is to generate a symmetric encryption key out of a
> > passphrase.  Is there a recommended way to do that using the algorithms
> > already implemented in the kernel?
> 
> For example, dmcrypt uses PBKDF2 for its operation. And this PBKDF is done in 
> user space by libcryptsetup that utilizes a crypto lib, commonly libgcrypt.

I know that.  We can do that here too in principle, but I'd prefer all crypto
to take place in the kernel in this particular case.

Thanks,
Rafael

Re: PBKDF2 support in the linux kernel

[Stephan Mueller]

Am Donnerstag, 24. Mai 2018, 10:33:07 CEST schrieb Rafael J. Wysocki:

Hi Rafael,

> So the problem is that Yu would like to use this for hibernation encryption
> done entirely in the kernel.

But why do you need to perform PBKDF in kernel space?

If you retain the password information in the kernel, you could retain the 
derived key instead of the passcode.

If, however, you ask for the user password during resume, you need some user 
space component to query that password. The PBKDF can also be handled in user 
space along with the query.

Or how do you want to handle the passcode?
> 
> The exact use case is to generate a symmetric encryption key out of a
> passphrase.  Is there a recommended way to do that using the algorithms
> already implemented in the kernel?

For example, dmcrypt uses PBKDF2 for its operation. And this PBKDF is done in 
user space by libcryptsetup that utilizes a crypto lib, commonly libgcrypt.
> 
> Thanks,
> Rafael



Ciao
Stephan

Re: PBKDF2 support in the linux kernel

[Rafael J. Wysocki]

On Thursday, May 24, 2018 7:57:37 AM CEST Stephan Mueller wrote:
> Am Donnerstag, 24. Mai 2018, 04:45:00 CEST schrieb Eric Biggers:
> 
> Hi Eric,
> 
> > 
> > "Not having to rely on any third-party library" is not an excuse to add
> > random code to the kernel, which runs in a privileged context.  Please do
> > PBKDF2 in userspace instead.
> 
> I second that. Besides, if you really need to rely on the kernel crypto API 
> to 
> do that because you do not want to add yet another crypto lib, libkcapi has a 
> PBKDF2 implementation that uses the kernel crypto API via AF_ALG. I.e. the 
> kernel crypto API is used and yet the PBKDF logic is in user space.
> 
> http://www.chronox.de/libkcapi.html

So the problem is that Yu would like to use this for hibernation encryption
done entirely in the kernel.

The exact use case is to generate a symmetric encryption key out of a
passphrase.  Is there a recommended way to do that using the algorithms
already implemented in the kernel?

Thanks,
Rafael

Re: PBKDF2 support in the linux kernel

[Stephan Mueller]

Am Donnerstag, 24. Mai 2018, 04:45:00 CEST schrieb Eric Biggers:

Hi Eric,

> 
> "Not having to rely on any third-party library" is not an excuse to add
> random code to the kernel, which runs in a privileged context.  Please do
> PBKDF2 in userspace instead.

I second that. Besides, if you really need to rely on the kernel crypto API to 
do that because you do not want to add yet another crypto lib, libkcapi has a 
PBKDF2 implementation that uses the kernel crypto API via AF_ALG. I.e. the 
kernel crypto API is used and yet the PBKDF logic is in user space.

http://www.chronox.de/libkcapi.html

Ciao
Stephan

Re: PBKDF2 support in the linux kernel

[Eric Biggers]

Hi Yu,

On Thu, May 24, 2018 at 10:26:12AM +0800, Yu Chen wrote:
> Hi Stephan,
> thanks for your reply,
> On Wed, May 23, 2018 at 1:43 AM Stephan Mueller  wrote:
> 
> > Am Dienstag, 22. Mai 2018, 05:00:40 CEST schrieb Yu Chen:
> 
> > Hi Yu,
> 
> > > Hi all,
> > > The request is that, we'd like to generate a symmetric key derived from
> > > user provided passphase(not rely on any third-party library). May I
> know if
> > > there is a PBKDF2(Password-Based Key Derivation Function 2) support in
> the
> > > kernel? (https://tools.ietf.org/html/rfc2898#5.2)
> > > We have hmac sha1 in the kernel, do we have plan to port/implement
> > > corresponding PBKDF2 in the kernel too?
> 
> > There is no PBKDF2 support in the kernel.
> 
> I saw that there's already a kdf implementation using SP800-56A
> in security/keys/dh.c, I think I can learn from that and  implement PDKDF2
> using similar code.
> > Ciao
> > Stephan
> Best,
> Yu

"Not having to rely on any third-party library" is not an excuse to add random
code to the kernel, which runs in a privileged context.  Please do PBKDF2 in
userspace instead.

- Eric

Re: PBKDF2 support in the linux kernel

[Yu Chen]

Hi Stephan,
thanks for your reply,
On Wed, May 23, 2018 at 1:43 AM Stephan Mueller  wrote:

> Am Dienstag, 22. Mai 2018, 05:00:40 CEST schrieb Yu Chen:

> Hi Yu,

> > Hi all,
> > The request is that, we'd like to generate a symmetric key derived from
> > user provided passphase(not rely on any third-party library). May I
know if
> > there is a PBKDF2(Password-Based Key Derivation Function 2) support in
the
> > kernel? (https://tools.ietf.org/html/rfc2898#5.2)
> > We have hmac sha1 in the kernel, do we have plan to port/implement
> > corresponding PBKDF2 in the kernel too?

> There is no PBKDF2 support in the kernel.

I saw that there's already a kdf implementation using SP800-56A
in security/keys/dh.c, I think I can learn from that and  implement PDKDF2
using similar code.
> Ciao
> Stephan
Best,
Yu

Re: PBKDF2 support in the linux kernel

[Stephan Mueller]

Am Dienstag, 22. Mai 2018, 05:00:40 CEST schrieb Yu Chen:

Hi Yu,

> Hi all,
> The request is that, we'd like to generate a symmetric key derived from
> user provided passphase(not rely on any third-party library). May I know if
> there is a PBKDF2(Password-Based Key Derivation Function 2) support in the
> kernel? (https://tools.ietf.org/html/rfc2898#5.2)
> We have hmac sha1 in the kernel, do we have plan to port/implement
> corresponding PBKDF2 in the kernel too?

There is no PBKDF2 support in the kernel.

Ciao
Stephan

Re: [PATCH RESEND 1/2] Add DOWNLOAD_FIRMWARE SEV command

[Natarajan, Janakarajan]

On 5/10/2018 12:28 PM, Borislav Petkov wrote:

Use a prefix for the subject pls:

Subject: [PATCH RESEND 1/2] crypto: ccp: Add DOWNLOAD_FIRMWARE SEV command

or

Subject: [PATCH RESEND 1/2] crypto/ccp: Add DOWNLOAD_FIRMWARE SEV command

or so.


Okay.




On Wed, May 09, 2018 at 11:18:27AM -0500, Janakarajan Natarajan wrote:

The DOWNLOAD_FIRMWARE command, added as of SEV API v0.15, allows the OS
to install SEV firmware newer than the currently active SEV firmware.

For the new SEV firmware to be applied it must:
* Pass the validation test performed by the existing firmware.
* Be of the same build or a newer build compared to the existing firmware.

For more information please refer to "Section 5.11 DOWNLOAD_FIRMWARE" of
https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf

Signed-off-by: Janakarajan Natarajan 
---
  drivers/crypto/ccp/psp-dev.c | 96 +++-
  drivers/crypto/ccp/psp-dev.h |  4 ++
  include/linux/psp-sev.h  | 12 ++
  3 files changed, 102 insertions(+), 10 deletions(-)

diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index d95ec52..1c78a2e 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -22,11 +22,17 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "sp-dev.h"

  #include "psp-dev.h"
  
+#define SEV_VERSION_CHECK(_maj, _min)			\

+   ((psp_master->api_major) >= _maj &&   \
+(psp_master->api_minor) >= _min)
+
  #define DEVICE_NAME   "sev"
+#define SEV_FW_FILE"amd-sev.fw"

Can we put this firmware in an amd folder like

amd/sev.fw

or so, like we to with the microcode?


Yes. I can do that.




/lib/firmware/amd-ucode/
├── microcode_amd.bin
├── microcode_amd.bin.asc
├── microcode_amd_fam15h.bin
├── microcode_amd_fam15h.bin.asc
├── microcode_amd_fam16h.bin
└── microcode_amd_fam16h.bin.asc

It is tidier this way in the fw directory.


  static DEFINE_MUTEX(sev_cmd_mutex);
  static struct sev_misc_dev *misc_dev;
@@ -112,6 +118,7 @@ static int sev_cmd_buffer_len(int cmd)
case SEV_CMD_RECEIVE_UPDATE_DATA:   return sizeof(struct 
sev_data_receive_update_data);
case SEV_CMD_RECEIVE_UPDATE_VMSA:   return sizeof(struct 
sev_data_receive_update_vmsa);
case SEV_CMD_LAUNCH_UPDATE_SECRET:  return sizeof(struct 
sev_data_launch_secret);
+   case SEV_CMD_DOWNLOAD_FIRMWARE: return sizeof(struct 
sev_data_download_firmware);
default:return 0;
}
  
@@ -378,6 +385,77 @@ void *psp_copy_user_blob(u64 __user uaddr, u32 len)

  }
  EXPORT_SYMBOL_GPL(psp_copy_user_blob);
  
+static int get_sev_api_version(void)

sev_get_api_version()

like the rest.


+{
+   struct sev_user_data_status *status;
+   int error, ret;
+
+   status = _master->status_cmd_buf;
+   ret = sev_platform_status(status, );
+   if (ret) {
+   dev_err(psp_master->dev,
+   "SEV: failed to get status. Error: %#x\n", error);
+   return 1;
+   }
+
+   psp_master->api_major = status->api_major;
+   psp_master->api_minor = status->api_minor;
+   psp_master->build = status->build;
+
+   return 0;
+}
+
+/* Don't fail if SEV FW couldn't be updated. Continue with existing SEV FW */
+static void sev_update_firmware(struct device *dev)
+{
+   struct sev_data_download_firmware *data;
+   const struct firmware *firmware;
+   int ret, error, order;
+   struct page *p;
+   u64 data_size;
+
+   ret = request_firmware(, SEV_FW_FILE, dev);
+   if (ret < 0)
+   return;
+
+   /*
+* SEV FW expects the physical address given to it to be 32
+* byte aligned. Memory allocated has structure placed at the
+* beginning followed by the firmware being passed to the SEV
+* FW. Allocate enough memory for data structure + alignment
+* padding + SEV FW.
+*/
+   data_size = ALIGN(sizeof(struct sev_data_download_firmware), 32);
+
+   order = get_order(firmware->size + data_size);
+   p = alloc_pages(GFP_KERNEL, order);
+   if (!p)
+   goto fw_err;
+
+   /*
+* Copy firmware data to a kernel allocated contiguous
+* memory region.
+*/
+   data = page_address(p);
+   memcpy(page_address(p) + data_size, firmware->data, firmware->size);
+
+   data->address = __psp_pa(page_address(p) + data_size);
+   data->len = firmware->size;
+
+   ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, );
+   if (ret) {
+   dev_dbg(dev, "Failed to update SEV firmware: %#x\n", error);
+   } else {
+   dev_info(dev, "SEV firmware update successful\n");
+   get_sev_api_version();

Call that function in the caller of sev_update_firmware() and in its
success path. For that, make sev_update_firmware() return success/error

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Mon, May 21, 2018 at 3:43 PM, Gilad Ben-Yossef  wrote:
> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>  wrote:
>> Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
>> does not distinguish between the absence of the clock property, and an
>> actual error in getting the clock, and never considers any error a failure
>> (incl. -PROBE_DEFER).
>>
>> As of_clk_get() returns -ENOENT for both a missing clock property and a
>> missing clock, you should use (devm_)clk_get() instead, and distinguish
>> between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>
> I was trying to do as you suggested but I didn't quite get what is the
> dev_id (2nd) parameter to devm_clk_get parameter is supposed to be.

It's the (optional) name of the clock, helpful in case there is more than one.
In your case, NULL is fine.

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] crypto: Mark MORUS SIMD glue as x86-specific

[Ondrej Mosnáček]

2018-05-18 23:01 GMT+02:00 Ondrej Mosnáček :
> From: Ondrej Mosnacek 
>
> Commit 56e8e57fc3a7 ("crypto: morus - Add common SIMD glue code for
> MORUS") accidetally consiedered the glue code to be usable by different
> architectures, but it seems to be only usable on x86.
>
> This patch moves it under arch/x86/crypto and adds 'depends on X86' to
> the Kconfig options.
>
> Reported-by: kbuild test robot 
> Signed-off-by: Ondrej Mosnacek 
> ---
>  arch/x86/crypto/Makefile | 3 +++
>  {crypto => arch/x86/crypto}/morus1280_glue.c | 4 ++--
>  {crypto => arch/x86/crypto}/morus640_glue.c  | 4 ++--
>  crypto/Kconfig   | 6 --
>  crypto/Makefile  | 2 --
>  5 files changed, 11 insertions(+), 8 deletions(-)
>  rename {crypto => arch/x86/crypto}/morus1280_glue.c (98%)
>  rename {crypto => arch/x86/crypto}/morus640_glue.c (98%)
>
> diff --git a/arch/x86/crypto/Makefile b/arch/x86/crypto/Makefile
> index 3813e7cdaada..48e731d782e9 100644
> --- a/arch/x86/crypto/Makefile
> +++ b/arch/x86/crypto/Makefile
> @@ -42,6 +42,9 @@ obj-$(CONFIG_CRYPTO_AEGIS128_AESNI_SSE2) += aegis128-aesni.o
>  obj-$(CONFIG_CRYPTO_AEGIS128L_AESNI_SSE2) += aegis128l-aesni.o
>  obj-$(CONFIG_CRYPTO_AEGIS256_AESNI_SSE2) += aegis256-aesni.o
>
> +obj-$(CONFIG_CRYPTO_MORUS640_GLUE) += morus640_glue.o
> +obj-$(CONFIG_CRYPTO_MORUS1280_GLUE) += morus1280_glue.o
> +
>  obj-$(CONFIG_CRYPTO_MORUS640_SSE2) += morus640-sse2.o
>  obj-$(CONFIG_CRYPTO_MORUS1280_SSE2) += morus1280-sse2.o
>
> diff --git a/crypto/morus1280_glue.c b/arch/x86/crypto/morus1280_glue.c
> similarity index 98%
> rename from crypto/morus1280_glue.c
> rename to arch/x86/crypto/morus1280_glue.c
> index ce1e5c34b09d..0dccdda1eb3a 100644
> --- a/crypto/morus1280_glue.c
> +++ b/arch/x86/crypto/morus1280_glue.c
> @@ -1,6 +1,6 @@
>  /*
>   * The MORUS-1280 Authenticated-Encryption Algorithm
> - *   Common glue skeleton
> + *   Common x86 SIMD glue skeleton
>   *
>   * Copyright (c) 2016-2018 Ondrej Mosnacek 
>   * Copyright (C) 2017-2018 Red Hat, Inc. All rights reserved.
> @@ -299,4 +299,4 @@ EXPORT_SYMBOL_GPL(cryptd_morus1280_glue_exit_tfm);
>
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Ondrej Mosnacek ");
> -MODULE_DESCRIPTION("MORUS-1280 AEAD mode -- glue for optimizations");
> +MODULE_DESCRIPTION("MORUS-1280 AEAD mode -- glue for x86 optimizations");
> diff --git a/crypto/morus640_glue.c b/arch/x86/crypto/morus640_glue.c
> similarity index 98%
> rename from crypto/morus640_glue.c
> rename to arch/x86/crypto/morus640_glue.c
> index c7e788cfaa29..7b58fe4d9bd1 100644
> --- a/crypto/morus640_glue.c
> +++ b/arch/x86/crypto/morus640_glue.c
> @@ -1,6 +1,6 @@
>  /*
>   * The MORUS-640 Authenticated-Encryption Algorithm
> - *   Common glue skeleton
> + *   Common x86 SIMD glue skeleton
>   *
>   * Copyright (c) 2016-2018 Ondrej Mosnacek 
>   * Copyright (C) 2017-2018 Red Hat, Inc. All rights reserved.
> @@ -295,4 +295,4 @@ EXPORT_SYMBOL_GPL(cryptd_morus640_glue_exit_tfm);
>
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Ondrej Mosnacek ");
> -MODULE_DESCRIPTION("MORUS-640 AEAD mode -- glue for optimizations");
> +MODULE_DESCRIPTION("MORUS-640 AEAD mode -- glue for x86 optimizations");
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 75f5efde9aa3..0c9883d60a51 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -341,7 +341,8 @@ config CRYPTO_MORUS640
>   Support for the MORUS-640 dedicated AEAD algorithm.
>
>  config CRYPTO_MORUS640_GLUE
> -   tristate "MORUS-640 AEAD algorithm (glue for SIMD optimizations)"
> +   tristate "MORUS-640 AEAD algorithm (glue for x86 SIMD optimizations)"
> +   depends on X86
> select CRYPTO_AEAD
> select CRYPTO_CRYPTD
> help
> @@ -363,7 +364,8 @@ config CRYPTO_MORUS1280
>   Support for the MORUS-1280 dedicated AEAD algorithm.
>
>  config CRYPTO_MORUS1280_GLUE
> -   tristate "MORUS-1280 AEAD algorithm (glue for SIMD optimizations)"
> +   tristate "MORUS-1280 AEAD algorithm (glue for x86 SIMD optimizations)"
> +   depends on X86
> select CRYPTO_AEAD
> select CRYPTO_CRYPTD
> help

I realized these options shouldn't be shown to the user and thus
should have no prompt text set. I will send a v2 that also removes the
prompts.

Regards,

Ondrej

> diff --git a/crypto/Makefile b/crypto/Makefile
> index 68a7c546460a..6d1d40eeb964 100644
> --- a/crypto/Makefile
> +++ b/crypto/Makefile
> @@ -91,8 +91,6 @@ obj-$(CONFIG_CRYPTO_AEGIS128L) += aegis128l.o
>  obj-$(CONFIG_CRYPTO_AEGIS256) += aegis256.o
>  obj-$(CONFIG_CRYPTO_MORUS640) += morus640.o
>  obj-$(CONFIG_CRYPTO_MORUS1280) += morus1280.o
> -obj-$(CONFIG_CRYPTO_MORUS640_GLUE) += morus640_glue.o
> -obj-$(CONFIG_CRYPTO_MORUS1280_GLUE) += morus1280_glue.o
>  obj-$(CONFIG_CRYPTO_PCRYPT) += pcrypt.o
>  

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
 wrote:

>
> Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
> does not distinguish between the absence of the clock property, and an
> actual error in getting the clock, and never considers any error a failure
> (incl. -PROBE_DEFER).
>
> As of_clk_get() returns -ENOENT for both a missing clock property and a
> missing clock, you should use (devm_)clk_get() instead, and distinguish
> between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>

I was trying to do as you suggested but I didn't quite get what is the
dev_id (2nd) parameter to devm_clk_get parameter is supposed to be.

I see what of_clk_get() is doing, so can replicate that but it seems
an over kill.

Any ideas?

Thanks,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH v2] fscrypt: log the crypto algorithm implementations

[Theodore Y. Ts'o]

On Fri, May 18, 2018 at 10:58:14AM -0700, Eric Biggers wrote:
> Log the crypto algorithm driver name for each fscrypt encryption mode on
> its first use, also showing a friendly name for the mode.
> 
> This will help people determine whether the expected implementations are
> being used.  In some cases we've seen people do benchmarks and reject
> using encryption for performance reasons, when in fact they used a much
> slower implementation of AES-XTS than was possible on the hardware.  It
> can make an enormous difference; e.g., AES-XTS on ARM is about 10x
> faster with the crypto extensions (AES instructions) than without.
> 
> This also makes it more obvious which modes are being used, now that
> fscrypt supports multiple combinations of modes.
> 
> Example messages (with default modes, on x86_64):
> 
> [   35.492057] fscrypt: AES-256-CTS-CBC using implementation 
> "cts(cbc-aes-aesni)"
> [   35.492171] fscrypt: AES-256-XTS using implementation "xts-aes-aesni"
> 
> Note: algorithms can be dynamically added to the crypto API, which can
> result in different implementations being used at different times.  But
> this is rare; for most users, showing the first will be good enough.
> 
> Signed-off-by: Eric Biggers 

Applied, thanks.

- Ted

Re: [PATCH v2] fscrypt: add Speck128/256 support

[Theodore Y. Ts'o]

On Mon, May 07, 2018 at 05:22:08PM -0700, Eric Biggers wrote:
> fscrypt currently only supports AES encryption.  However, many low-end
> mobile devices have older CPUs that don't have AES instructions, e.g.
> the ARMv8 Cryptography Extensions.  Currently, user data on such devices
> is not encrypted at rest because AES is too slow, even when the NEON
> bit-sliced implementation of AES is used.  Unfortunately, it is
> infeasible to encrypt these devices at all when AES is the only option.
> 
> Therefore, this patch updates fscrypt to support the Speck block cipher,
> which was recently added to the crypto API.  The C implementation of
> Speck is not especially fast, but Speck can be implemented very
> efficiently with general-purpose vector instructions, e.g. ARM NEON.
> For example, on an ARMv7 processor, we measured the NEON-accelerated
> Speck128/256-XTS at 69 MB/s for both encryption and decryption, while
> AES-256-XTS with the NEON bit-sliced implementation was only 22 MB/s
> encryption and 19 MB/s decryption.
> 
> There are multiple variants of Speck.  This patch only adds support for
> Speck128/256, which is the variant with a 128-bit block size and 256-bit
> key size -- the same as AES-256.  This is believed to be the most secure
> variant of Speck, and it's only about 6% slower than Speck128/128.
> Speck64/128 would be at least 20% faster because it has 20% rounds, and
> it can be even faster on CPUs that can't efficiently do the 64-bit
> operations needed for Speck128.  However, Speck64's 64-bit block size is
> not preferred security-wise.  ARM NEON also supports the needed 64-bit
> operations even on 32-bit CPUs, resulting in Speck128 being fast enough
> for our targeted use cases so far.
> 
> The chosen modes of operation are XTS for contents and CTS-CBC for
> filenames.  These are the same modes of operation that fscrypt defaults
> to for AES.  Note that as with the other fscrypt modes, Speck will not
> be used unless userspace chooses to use it.  Nor are any of the existing
> modes (which are all AES-based) being removed, of course.
> 
> We intentionally don't make CONFIG_FS_ENCRYPTION select
> CONFIG_CRYPTO_SPECK, so people will have to enable Speck support
> themselves if they need it.  This is because we shouldn't bloat the
> FS_ENCRYPTION dependencies with every new cipher, especially ones that
> aren't recommended for most users.  Moreover, CRYPTO_SPECK is just the
> generic implementation, which won't be fast enough for many users; in
> practice, they'll need to enable CRYPTO_SPECK_NEON to get acceptable
> performance.
> 
> More details about our choice of Speck can be found in our patches that
> added Speck to the crypto API, and the follow-on discussion threads.
> We're planning a publication that explains the choice in more detail.
> But briefly, we can't use ChaCha20 as we previously proposed, since it
> would be insecure to use a stream cipher in this context, with potential
> IV reuse during writes on f2fs and/or on wear-leveling flash storage.
> 
> We also evaluated many other lightweight and/or ARX-based block ciphers
> such as Chaskey-LTS, RC5, LEA, CHAM, Threefish, RC6, NOEKEON, SPARX, and
> XTEA.  However, all had disadvantages vs. Speck, such as insufficient
> performance with NEON, much less published cryptanalysis, or an
> insufficient security level.  Various design choices in Speck make it
> perform better with NEON than competing ciphers while still having a
> security margin similar to AES, and in the case of Speck128 also the
> same available security levels.  Unfortunately, Speck does have some
> political baggage attached -- it's an NSA designed cipher, and was
> rejected from an ISO standard (though for context, as far as I know none
> of the above-mentioned alternatives are ISO standards either).
> Nevertheless, we believe it is a good solution to the problem from a
> technical perspective.
> 
> Certain algorithms constructed from ChaCha or the ChaCha permutation,
> such as MEM (Masked Even-Mansour) or HPolyC, may also meet our
> performance requirements.  However, these are new constructions that
> need more time to receive the cryptographic review and acceptance needed
> to be confident in their security.  HPolyC hasn't been published yet,
> and we are concerned that MEM makes stronger assumptions about the
> underlying permutation than the ChaCha stream cipher does.  In contrast,
> the XTS mode of operation is relatively well accepted, and Speck has
> over 70 cryptanalysis papers.  Of course, these ChaCha-based algorithms
> can still be added later if they become ready.
> 
> The best known attack on Speck128/256 is a differential cryptanalysis
> attack on 25 of 34 rounds with 2^253 time complexity and 2^125 chosen
> plaintexts, i.e. only marginally faster than brute force.  There is no
> known attack on the full 34 rounds.
> 
> Signed-off-by: Eric Biggers 

Applied, thanks.

- Ted

Re: cryptomgr_test / drbg_ctr: BUG: sleeping function called from invalid context

[Stephan Müller]

Am Freitag, 18. Mai 2018, 10:36:04 CEST schrieb Geert Uytterhoeven:

Hi Geert,
> 
> I tried following the code path, but couldn't find where it went wrong.
> 
> mutex_lock(>drbg_mutex) is called from drbg_instantiate(), which is
> inlined by the compiler into drbg_kcapi_seed().
> 
> Do you have a clue?

It is the first time I hear from such an issue. Yes, the DRBG should not be 
called in atomic context. But I do not see where we have an atomic context 
(either a spin_lock or in an interrupt handler) when we are executing the test 
manager.

I will keep looking.

Ciao
Stephan

Re: [PATCH 3/3] crypto: x86 - Add optimized AEGIS implementations

[Ondrej Mosnáček]

2018-05-20 4:41 GMT+02:00 Eric Biggers :
> Hi Ondrej,
>
> On Fri, May 11, 2018 at 02:12:51PM +0200, Ondrej Mosnáček wrote:
>> From: Ondrej Mosnacek 
>>
>> This patch adds optimized implementations of AEGIS-128, AEGIS-128L,
>> and AEGIS-256, utilizing the AES-NI and SSE2 x86 extensions.
>>
>> Signed-off-by: Ondrej Mosnacek 
> [...]
>> +static int crypto_aegis256_aesni_setkey(struct crypto_aead *aead, const u8 
>> *key,
>> + unsigned int keylen)
>> +{
>> + struct aegis_ctx *ctx = crypto_aegis256_aesni_ctx(aead);
>> +
>> + if (keylen != AEGIS256_KEY_SIZE) {
>> + crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
>> + return -EINVAL;
>> + }
>> +
>> + memcpy(ctx->key.bytes, key, AEGIS256_KEY_SIZE);
>> +
>> + return 0;
>> +}
>
> This code is copying 32 bytes into a 16-byte buffer.

Indeed, I must have overlooked that while copy-pasting and editing the
boilerplate...

I will send a follow-up patch soon.

Thanks for the report!

>
> ==
> BUG: KASAN: slab-out-of-bounds in memcpy include/linux/string.h:345 [inline]
> BUG: KASAN: slab-out-of-bounds in crypto_aegis256_aesni_setkey+0x23/0x60 
> arch/x86/crypto/aegis256-aesni-glue.c:167
> Write of size 32 at addr 88006c16b650 by task cryptomgr_test/120
> CPU: 2 PID: 120 Comm: cryptomgr_test Not tainted 
> 4.17.0-rc1-00069-g6ecc9d9ff91f #31
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> 1.11.0-20171110_100015-anatol 04/01/2014
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x86/0xca lib/dump_stack.c:113
>  print_address_description+0x65/0x204 mm/kasan/report.c:256
>  kasan_report_error mm/kasan/report.c:354 [inline]
>  kasan_report.cold.6+0x242/0x304 mm/kasan/report.c:412
>  check_memory_region_inline mm/kasan/kasan.c:260 [inline]
>  check_memory_region+0x13c/0x1b0 mm/kasan/kasan.c:267
>  memcpy+0x37/0x50 mm/kasan/kasan.c:303
>  memcpy include/linux/string.h:345 [inline]
>  crypto_aegis256_aesni_setkey+0x23/0x60 
> arch/x86/crypto/aegis256-aesni-glue.c:167
>  crypto_aead_setkey+0xa4/0x1e0 crypto/aead.c:62
>  cryptd_aead_setkey+0x30/0x50 crypto/cryptd.c:938
>  crypto_aead_setkey+0xa4/0x1e0 crypto/aead.c:62
>  cryptd_aegis256_aesni_setkey+0x30/0x50 
> arch/x86/crypto/aegis256-aesni-glue.c:259
>  crypto_aead_setkey+0xa4/0x1e0 crypto/aead.c:62
>  __test_aead+0x8bf/0x3770 crypto/testmgr.c:675
>  test_aead+0x28/0x110 crypto/testmgr.c:957
>  alg_test_aead+0x8b/0x140 crypto/testmgr.c:1690
>  alg_test.part.5+0x1bb/0x4d0 crypto/testmgr.c:3845
>  alg_test+0x23/0x25 crypto/testmgr.c:3865
>  cryptomgr_test+0x56/0x80 crypto/algboss.c:223
>  kthread+0x329/0x3f0 kernel/kthread.c:238
>  ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:412
> Allocated by task 120:
>  save_stack mm/kasan/kasan.c:448 [inline]
>  set_track mm/kasan/kasan.c:460 [inline]
>  kasan_kmalloc.part.1+0x5f/0xf0 mm/kasan/kasan.c:553
>  kasan_kmalloc+0xaf/0xc0 mm/kasan/kasan.c:538
>  __do_kmalloc mm/slab.c:3718 [inline]
>  __kmalloc+0x114/0x1d0 mm/slab.c:3727
>  kmalloc include/linux/slab.h:517 [inline]
>  kzalloc include/linux/slab.h:701 [inline]
>  crypto_create_tfm+0x80/0x2c0 crypto/api.c:464
>  crypto_spawn_tfm2+0x57/0x90 crypto/algapi.c:717
>  crypto_spawn_aead include/crypto/internal/aead.h:112 [inline]
>  cryptd_aead_init_tfm+0x3d/0x110 crypto/cryptd.c:1033
>  crypto_aead_init_tfm+0x130/0x190 crypto/aead.c:111
>  crypto_create_tfm+0xda/0x2c0 crypto/api.c:471
>  crypto_alloc_tfm+0xcf/0x1d0 crypto/api.c:543
>  crypto_alloc_aead+0x14/0x20 crypto/aead.c:351
>  cryptd_alloc_aead+0xeb/0x1c0 crypto/cryptd.c:1334
>  cryptd_aegis256_aesni_init_tfm+0x24/0xf0 
> arch/x86/crypto/aegis256-aesni-glue.c:308
>  crypto_aead_init_tfm+0x130/0x190 crypto/aead.c:111
>  crypto_create_tfm+0xda/0x2c0 crypto/api.c:471
>  crypto_alloc_tfm+0xcf/0x1d0 crypto/api.c:543
>  crypto_alloc_aead+0x14/0x20 crypto/aead.c:351
>  alg_test_aead+0x1f/0x140 crypto/testmgr.c:1682
>  alg_test.part.5+0x1bb/0x4d0 crypto/testmgr.c:3845
>  alg_test+0x23/0x25 crypto/testmgr.c:3865
>  cryptomgr_test+0x56/0x80 crypto/algboss.c:223
>  kthread+0x329/0x3f0 kernel/kthread.c:238
>  ret_from_[   16.453502] serial8250: too much work for irq4
> Freed by task 0:
> (stack is not available)
> The buggy address belongs to the object at 88006c16b600
> The buggy address is located 80 bytes inside of
> The buggy address belongs to the page:
> page:ea00017a4f68 count:1 mapcount:0 mapping:88006c16b000 index:0x0
> flags: 0x1000100(slab)
> raw: 01000100 88006c16b000  00010015
> raw: ea00017a2470 88006d401548 88006d400400
> page dumped because: kasan: bad access detected
> Memory state around the buggy address:
>  88006c16b500: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  88006c16b580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc

Re: [PATCH 3/3] crypto: x86 - Add optimized AEGIS implementations

[Eric Biggers]

Hi Ondrej,

On Fri, May 11, 2018 at 02:12:51PM +0200, Ondrej Mosnáček wrote:
> From: Ondrej Mosnacek 
> 
> This patch adds optimized implementations of AEGIS-128, AEGIS-128L,
> and AEGIS-256, utilizing the AES-NI and SSE2 x86 extensions.
> 
> Signed-off-by: Ondrej Mosnacek 
[...]
> +static int crypto_aegis256_aesni_setkey(struct crypto_aead *aead, const u8 
> *key,
> + unsigned int keylen)
> +{
> + struct aegis_ctx *ctx = crypto_aegis256_aesni_ctx(aead);
> +
> + if (keylen != AEGIS256_KEY_SIZE) {
> + crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> + return -EINVAL;
> + }
> +
> + memcpy(ctx->key.bytes, key, AEGIS256_KEY_SIZE);
> +
> + return 0;
> +}

This code is copying 32 bytes into a 16-byte buffer.

==
BUG: KASAN: slab-out-of-bounds in memcpy include/linux/string.h:345 [inline]
BUG: KASAN: slab-out-of-bounds in crypto_aegis256_aesni_setkey+0x23/0x60 
arch/x86/crypto/aegis256-aesni-glue.c:167
Write of size 32 at addr 88006c16b650 by task cryptomgr_test/120
CPU: 2 PID: 120 Comm: cryptomgr_test Not tainted 4.17.0-rc1-00069-g6ecc9d9ff91f 
#31
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.11.0-20171110_100015-anatol 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x86/0xca lib/dump_stack.c:113
 print_address_description+0x65/0x204 mm/kasan/report.c:256
 kasan_report_error mm/kasan/report.c:354 [inline]
 kasan_report.cold.6+0x242/0x304 mm/kasan/report.c:412
 check_memory_region_inline mm/kasan/kasan.c:260 [inline]
 check_memory_region+0x13c/0x1b0 mm/kasan/kasan.c:267
 memcpy+0x37/0x50 mm/kasan/kasan.c:303
 memcpy include/linux/string.h:345 [inline]
 crypto_aegis256_aesni_setkey+0x23/0x60 
arch/x86/crypto/aegis256-aesni-glue.c:167
 crypto_aead_setkey+0xa4/0x1e0 crypto/aead.c:62
 cryptd_aead_setkey+0x30/0x50 crypto/cryptd.c:938
 crypto_aead_setkey+0xa4/0x1e0 crypto/aead.c:62
 cryptd_aegis256_aesni_setkey+0x30/0x50 
arch/x86/crypto/aegis256-aesni-glue.c:259
 crypto_aead_setkey+0xa4/0x1e0 crypto/aead.c:62
 __test_aead+0x8bf/0x3770 crypto/testmgr.c:675
 test_aead+0x28/0x110 crypto/testmgr.c:957
 alg_test_aead+0x8b/0x140 crypto/testmgr.c:1690
 alg_test.part.5+0x1bb/0x4d0 crypto/testmgr.c:3845
 alg_test+0x23/0x25 crypto/testmgr.c:3865
 cryptomgr_test+0x56/0x80 crypto/algboss.c:223
 kthread+0x329/0x3f0 kernel/kthread.c:238
 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:412
Allocated by task 120:
 save_stack mm/kasan/kasan.c:448 [inline]
 set_track mm/kasan/kasan.c:460 [inline]
 kasan_kmalloc.part.1+0x5f/0xf0 mm/kasan/kasan.c:553
 kasan_kmalloc+0xaf/0xc0 mm/kasan/kasan.c:538
 __do_kmalloc mm/slab.c:3718 [inline]
 __kmalloc+0x114/0x1d0 mm/slab.c:3727
 kmalloc include/linux/slab.h:517 [inline]
 kzalloc include/linux/slab.h:701 [inline]
 crypto_create_tfm+0x80/0x2c0 crypto/api.c:464
 crypto_spawn_tfm2+0x57/0x90 crypto/algapi.c:717
 crypto_spawn_aead include/crypto/internal/aead.h:112 [inline]
 cryptd_aead_init_tfm+0x3d/0x110 crypto/cryptd.c:1033
 crypto_aead_init_tfm+0x130/0x190 crypto/aead.c:111
 crypto_create_tfm+0xda/0x2c0 crypto/api.c:471
 crypto_alloc_tfm+0xcf/0x1d0 crypto/api.c:543
 crypto_alloc_aead+0x14/0x20 crypto/aead.c:351
 cryptd_alloc_aead+0xeb/0x1c0 crypto/cryptd.c:1334
 cryptd_aegis256_aesni_init_tfm+0x24/0xf0 
arch/x86/crypto/aegis256-aesni-glue.c:308
 crypto_aead_init_tfm+0x130/0x190 crypto/aead.c:111
 crypto_create_tfm+0xda/0x2c0 crypto/api.c:471
 crypto_alloc_tfm+0xcf/0x1d0 crypto/api.c:543
 crypto_alloc_aead+0x14/0x20 crypto/aead.c:351
 alg_test_aead+0x1f/0x140 crypto/testmgr.c:1682
 alg_test.part.5+0x1bb/0x4d0 crypto/testmgr.c:3845
 alg_test+0x23/0x25 crypto/testmgr.c:3865
 cryptomgr_test+0x56/0x80 crypto/algboss.c:223
 kthread+0x329/0x3f0 kernel/kthread.c:238
 ret_from_[   16.453502] serial8250: too much work for irq4
Freed by task 0:
(stack is not available)
The buggy address belongs to the object at 88006c16b600
The buggy address is located 80 bytes inside of
The buggy address belongs to the page:
page:ea00017a4f68 count:1 mapcount:0 mapping:88006c16b000 index:0x0
flags: 0x1000100(slab)
raw: 01000100 88006c16b000  00010015
raw: ea00017a2470 88006d401548 88006d400400
page dumped because: kasan: bad access detected
Memory state around the buggy address:
 88006c16b500: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 88006c16b580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>88006c16b600: 00 00 00 00 00 00 00 00 00 00 00 00 00 fc fc fc
  ^
 88006c16b680: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
 88006c16b700: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
==
Disabling lock debugging due to kernel taint

Re: [PATCH 0/4] Add support for MORUS AEAD algorithm

[Herbert Xu]

On Fri, May 11, 2018 at 02:19:08PM +0200, Ondrej Mosnáček wrote:
> From: Ondrej Mosnacek 
> 
> This patchset adds the MORUS AEAD algorithm implementation to the Linux 
> Crypto API.
> 
> MORUS [1] is a dedicated AEAD algorithm focused on SIMD instructions and 
> designed for high throughput both on modern processors and in hardware. It is 
> designed by Hongjun Wu and Tao Huang and has been submitted to the CAESAR 
> competiton [2], where it is currently one of the finalists [3]. MORUS uses 
> only logical bitwise operations and bitwise rotations as primitives.
> 
> MORUS has two variants:
> * MORUS-640 operating on 128-bit blocks and accepting a 128-bit key.
> * MORUS-1280 operating on 256-bit blocks and accepting a 128- or 256-bit key.
> Both variants accept a 128-bit IV and produce an up to 128-bit tag.
> 
> The patchset contains four patches, adding:
> * generic implementations
> * test vectors to testmgr
> * common glue code for x86_64 optimizations
> * x86_64 SSE2/AVX2 optimized implementations
> 
> Since there are no official test vectors currently available, the test 
> vectors in patch 2 were generated using a reference implementation from 
> public CAESAR benchmarks [4]. They should be replaced/complemented with 
> official test vectors if/when they become available.
> 
> The implementations have been developed in cooperation with Milan Broz (the 
> maintainer of dm-crypt and cryptsetup) and there is a plan to use them for 
> authenticated disk encryption in cryptsetup. They are a result of my Master's 
> thesis at the Faculty of Informatics, Masaryk University, Brno [5].
> 
> [1] https://competitions.cr.yp.to/round3/morusv2.pdf
> [2] https://competitions.cr.yp.to/caesar-call.html
> [3] https://competitions.cr.yp.to/caesar-submissions.html
> [4] https://bench.cr.yp.to/ebaead.html
> [5] https://is.muni.cz/th/409879/fi_m/?lang=en
> 
> Ondrej Mosnacek (4):
>   crypto: Add generic MORUS AEAD implementations
>   crypto: testmgr - Add test vectors for MORUS
>   crypto: Add common SIMD glue code for MORUS
>   crypto: x86 - Add optimized MORUS implementations

All applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 0/3] Add support for AEGIS AEAD algorithm

[Herbert Xu]

On Fri, May 11, 2018 at 02:12:48PM +0200, Ondrej Mosnáček wrote:
> From: Ondrej Mosnacek 
> 
> This patchset adds the AEGIS AEAD algorithm implementation to the Linux 
> Crypto API.
> 
> AEGIS [1] is a dedicated AEAD algorithm based on the AES round function and 
> designed for high throughput both on modern processors and in hardware. It is 
> designed by Hongjun Wu and Bart Preneel and has been submitted to the CAESAR 
> competiton [2], where it is currently one of the finalists [3].
> 
> AEGIS uses the AES round function and logical bitwise operations as 
> primitives. It achieves extremely good performance in software (on platforms 
> with HW-accelerated AES round function) and in hardware.
> 
> AEGIS has three variants:
> * AEGIS-128 operating on 128-bit blocks and accepting a 128-bit IV and key.
> * AEGIS-128L operating on pairs of 128-bit blocks and accepting a 128-bit IV 
> and key.
> * AEGIS-256 operating on 128-bit blocks and accepting a 256-bit IV and key.
> All three variants produce an up to 128-bit tag.
> 
> The patchset contains three patches, adding:
> * generic implementations
> * test vectors to testmgr
> * x86_64 AES-NI+SSE2 optimized implementations
> 
> Since there are no official test vectors currently available, the test 
> vectors in patch 2 were generated using a reference implementation from 
> public CAESAR benchmarks [4]. They should be replaced/complemented with 
> official test vectors if/when they become available.
> 
> The implementations have been developed in cooperation with Milan Broz (the 
> maintainer of dm-crypt and cryptsetup) and there is a plan to use them for 
> authenticated disk encryption in cryptsetup. They are a result of my Master's 
> thesis at the Faculty of Informatics, Masaryk University, Brno [5].
> 
> [1] https://competitions.cr.yp.to/round3/aegisv11.pdf
> [2] https://competitions.cr.yp.to/caesar-call.html
> [3] https://competitions.cr.yp.to/caesar-submissions.html
> [4] https://bench.cr.yp.to/ebaead.html
> [5] https://is.muni.cz/th/409879/fi_m/?lang=en
> 
> Ondrej Mosnacek (3):
>   crypto: Add generic AEGIS AEAD implementations
>   crypto: testmgr - Add test vectors for AEGIS
>   crypto: x86 - Add optimized AEGIS implementations

All applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: reorder paes test lexicographically

[Herbert Xu]

On Fri, May 11, 2018 at 09:04:06AM +0100, Gilad Ben-Yossef wrote:
> Due to a snafu "paes" testmgr tests were not ordered
> lexicographically, which led to boot time warnings.
> Reorder the tests as needed.
> 
> Fixes: a794d8d ("crypto: ccree - enable support for hardware keys")
> Reported-by: Abdul Haleem 
> Signed-off-by: Gilad Ben-Yossef 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: nx: fix spelling mistake: "seqeunce" -> "sequence"

[Herbert Xu]

On Wed, May 09, 2018 at 10:16:36AM +0100, Colin King wrote:
> From: Colin Ian King 
> 
> Trivial fix to spelling mistake in CSB_ERR error message text
> 
> Signed-off-by: Colin Ian King 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: chelsio: request to HW should wrap

[Herbert Xu]

On Thu, May 10, 2018 at 10:14:42AM +0530, Atul Gupta wrote:
> -Tx request and data is copied to HW Q in 64B desc, check for
> end of queue and adjust the current position to start from
> beginning before passing the additional request info.
> -key context copy should check key length only
> -Few reverse christmas tree correction
> 
> Signed-off-by: Atul Gupta 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] hwrng: n2: fix spelling mistake: "restesting" -> "retesting"

[Herbert Xu]

Colin King  wrote:
> From: Colin Ian King 
> 
> Trivial fix to spelling mistake in dev_err error message
> 
> Signed-off-by: Colin Ian King 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH 5/5] KEYS: add KPP ecdh parser

[Tudor Ambarus]

Hi, Denis,

On 05/14/2018 10:54 PM, Denis Kenzior wrote:

Hi Tudor,

On 02/28/2018 10:52 AM, Tudor Ambarus wrote:

The ECDH private keys are expected to be encoded with the ecdh
helpers from kernel.

Use the ecdh helpers to check if the key is valid. If valid,
allocate a tfm and set the private key. There is a one-to-one
binding between the private key and the tfm. The tfm is allocated
once and used as many times as needed.

ECDH keys can be loaded like this:
 # echo -n 
020028000200200024d121ebe5cf2d83f6621b6e43843aa38be086c32019da92505303e1c0eab882 
\


This part looks a bit scary.  Isn't this translating directly to 
kpp_secret data structure (in looks like little-endian order) followed 
curve_id, etc. >


yes, this is how it works.

If the intent is to extend KPP with regular DH, DH + KDF, etc, then we 
might want to invent a proper format here?  I don't think that a Diffie 
Hellman or ECDH Private Key format was ever invented, similar to how 
PKCS8 is used for RSA.




This can be resolved by falling through kpp decoding types until one
recognizes the format.

Inventing an ASN.1 syntax would be logical but somewhat painful as D-H 
is frequently used with plain old random numbers and certificates are 
not stored on disk...


There was this kind of discussion when kpp was introduced, see:
https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg19599.html

Best,
ta




 | xxd -r -p | keyctl padd asymmetric private @s

Signed-off-by: Tudor Ambarus 
---
  crypto/asymmetric_keys/Kconfig  |   8 +++
  crypto/asymmetric_keys/Makefile |   5 ++
  crypto/asymmetric_keys/kpp_parser.c | 124 


  include/crypto/asym_kpp_subtype.h   |   2 +
  4 files changed, 139 insertions(+)
  create mode 100644 crypto/asymmetric_keys/kpp_parser.c


Regards,
-Denis

Re: [RFC PATCH 1/5] KEYS: Provide key type operations for kpp ops

[Tudor Ambarus]

Hi, Denis,

Thanks for the review! Please see inline.

On 05/14/2018 09:48 PM, Denis Kenzior wrote:

Hi Tudor,

On 02/28/2018 10:52 AM, Tudor Ambarus wrote:

Provide three new operations in the key_type struct that can be used to
provide access to kpp operations. These will be implemented for the
asymmetric key type in a later patch and may refer to a key retained in
RAM by the kernel or a key retained in crypto hardware.

 int (*asym_kpp_query)(const struct kernel_kpp_params *params,
   struct kernel_kpp_query *res);
 int (*asym_kpp_gen_pubkey)(struct kernel_kpp_params *params,
   void *out);
 int (*asym_kpp_compute_ss)(struct kernel_kpp_params *params,
const void *in, void *out);

Signed-off-by: Tudor Ambarus 
---
  Documentation/security/keys/core.rst | 54 


  include/linux/key-type.h |  7 +
  include/linux/keyctl.h   | 11 
  include/uapi/linux/keyctl.h  |  3 ++
  4 files changed, 75 insertions(+)

diff --git a/Documentation/security/keys/core.rst 
b/Documentation/security/keys/core.rst

index d224fd0..9b69a1f 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -1688,6 +1688,60 @@ The structure has a number of fields, some of 
which are mandatory:
   If successful, 0 will be returned.  If the key doesn't support 
this,

   EOPNOTSUPP will be returned.
+  *  ``int (*asym_kpp_gen_pubkey)(struct kernel_kpp_params *params, 
void *out);``
+  *  ``int (*asym_kpp_compute_ss)(struct kernel_kpp_params *params, 
const void *in, void *out);``

+
+ These methods are optional. If provided the first allows to 
generate the
+ public key pair corresponding to the private key. The second 
method allows
+ to generate a shared secret by  combining the private key and 
the other

+ party's public key.
+
+ In all cases, the following information is provided in the 
params block::

+
+   struct kernel_kpp_query {
+   struct key  *key;
+   __u32   in_len; /* Input data size */
+   __u32   out_len;/* Output buffer size */
+   }
+


Probably not a huge deal as most common key sizes are already supported, 
but... is there a way to query supported key sizes?  I think for 
DH_COMPUTE we didn't have this problem as everything was done in 
software.  However, if the intent is to use TPM / other hardware engines 
we might need a way to query supported key sizes.


Oh, there's a typo here, this structure should have been named
'kernel_kpp_params' and not 'kernel_kpp_query'. 'kernel_kpp_query' is
defined below, it provides a way for determining the maximum supported
key size.



+ This includes the key to be used and the sizes in bytes of the 
input and

+ output buffers.
+
+ For a given operation, the in and out buffers are used as follows::
+
+   Operation IDin,in_lenout,out_len
+   === ===  

+   KEYCTL_KPP_GEN_PUBKEY   -Corresponding 
public key

+   KEYCTL_KPP_COMPUTE_SS   Pair's public keyShared Secret
+
+ If successful, the public key generation and the shared secret 
computation

+ will return the amount of data written into the output buffer.
+
+  *  ``int (*asym_kpp_query)(const struct kernel_kpp_params *params, 
struct kernel_kpp_query *res);``

+
+ This method is optional. If provided it allows information about 
the

+ asymmetric KPP (Key Protocol Primitives) key held in the key to be
+ determined.
+
+ The ``params`` block will contain the key to be queried. 
``in_len`` and

+ ``out_len`` are unused.
+
+ If successful, the following information is filled in::
+
+   struct kernel_kpp_query {
+   __u32   supported_ops;  /* Which ops are 
supported */
+   __u32   max_size;   /* Maximum size of the 
output buffer */

+   };
+
+ The supported_ops field will contain a bitmask indicating what 
operations
+ are supported by the key, including public key generation and 
shared

+ secret computation. The following constants are defined for this::
+
+   KEYCTL_SUPPORTS_{GEN_PUBKEY, COMPUTE_SS};
+
+ If successful, 0 is returned.  If the key is not an asymmetric 
kpp key,

+ EOPNOTSUPP is returned.
+
  Request-Key Callback Service
  
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index bc9af55..d354b6b 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -19,6 +19,8 @@
  struct kernel_pkey_query;
  struct kernel_pkey_params;
+struct kernel_kpp_query;
+struct kernel_kpp_params;
  /*
   * key under-construction record
@@ -165,6 +167,11 @@ struct key_type {
 const void *in, void 

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Simon Horman]

On Thu, May 17, 2018 at 04:12:23PM +0300, Gilad Ben-Yossef wrote:
> On Thu, May 17, 2018 at 12:04 PM, Simon Horman  wrote:
> > On Thu, May 17, 2018 at 11:01:57AM +0300, Gilad Ben-Yossef wrote:
> >> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
> >> > On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
> >> >> Hi Gilad,
> >> >>
> >> >> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
> >> >> wrote:
> >> >> > Add bindings for CryptoCell instance in the SoC.
> >> >> >
> >> >> > Signed-off-by: Gilad Ben-Yossef 
> >> >>
> >> >> Thanks for your patch!
> >> >>
> >> >> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> >> >> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> >> >> > @@ -528,6 +528,14 @@
> >> >> > status = "disabled";
> >> >> > };
> >> >> >
> >> >> > +   arm_cc630p: crypto@e6601000 {
> >> >> > +   compatible = "arm,cryptocell-630p-ree";
> >> >> > +   interrupts = ;
> >> >> > +   #interrupt-cells = <2>;
> >> >>
> >> >> I believe the #interrupt-cells property is not needed.
> >> >>
> >> >> > +   reg = <0x0 0xe6601000 0 0x1000>;
> >> >> > +   clocks = < CPG_MOD 229>;
> >> >> > +   };
> >> >>
> >> >> The rest looks good, but I cannot verify the register block.
> >> >>
> >> >> > +
> >> >> > i2c3: i2c@e66d {
> >> >> > #address-cells = <1>;
> >> >> > #size-cells = <0>;
> >> >
> >> > Thanks, I have applied this after dropping the #interrupt-cells property.
> >>
> >> Thanks you!
> >>
> >> Alas, it will not work without the clk patch (the previous one in the
> >> series) so they need to be
> >> taken or dropped together.
> >
> > I think its fine if it does not yet work.
> > But not if its causes things that previously worked to stop working.
> 
> Based on the further discussion with Geert my recommendation is to
> drop my patch for now,
> take Geert CR clock  patch and I will follow up next week with a v2
> that fixes the clock
> handing as discussed with Geert.

Thanks, I will drop the patch.

Re: [PATCH 1/5] random: fix crng_ready() test

[Theodore Y. Ts'o]

On Thu, May 17, 2018 at 08:01:04AM +0200, Christophe LEROY wrote:
> 
> On a powerpc embedded board which has an mpc8xx processor running at 133Mhz,
> I now get the startup done in more than 7 minutes instead of 30 seconds.
> This is due to the webserver blocking on read on /dev/random until we get
> 'random: crng init done':
> 
> [0.00] Linux version 4.17.0-rc4-00415-gd2f75d40072d (root@localhost)
> (gcc version 5.4.0 (GCC)) #203 PREEMPT Wed May 16 16:32:02 CEST 2018
> [0.295453] random: get_random_u32 called from
> bucket_table_alloc+0x84/0x1bc with crng_init=0
> [1.030472] device: 'random': device_add
> [1.031279] device: 'urandom': device_add
> [1.420069] device: 'hw_random': device_add
> [2.156853] random: fast init done
> [  462.007776] random: crng init done
> 
> This has become really critical, is there anything that can be done ?

Figure out why the webserver needs to read /dev/random and is it for a
security critical purpose?

A kernel patch which makes the kernel do a "lalalalala I'm secure"
when it really isn't secure is a simple "solution", but would it
really make you happy?


- Ted

Re: [PATCH 1/5] random: fix crng_ready() test

[Theodore Y. Ts'o]

On Wed, May 16, 2018 at 05:07:08PM -0700, Srivatsa S. Bhat wrote:
> 
> On a Photon OS VM running on VMware ESXi, this patch causes a boot speed
> regression of 5 minutes :-( [ The VM doesn't have haveged or rng-tools
> (rngd) installed. ]
> 
> [1.420246] EXT4-fs (sda2): re-mounted. Opts: barrier,noacl,data=ordered
> [1.469722] tsc: Refined TSC clocksource calibration: 1900.002 MHz
> [1.470707] clocksource: tsc: mask: 0x max_cycles: 
> 0x36c65c1a9e1, max_idle_ns: 881590695311 ns
> [1.474249] clocksource: Switched to clocksource tsc
> [1.584427] systemd-journald[216]: Received request to flush runtime 
> journal from PID 1
> [  346.620718] random: crng init done
> 
> Interestingly, the boot delay is exacerbated on VMs with large amounts
> of RAM. For example, the delay is not so noticeable (< 30 seconds) on a
> VM with 2GB memory, but goes up to 5 minutes on an 8GB VM.
> 
> Also, cloud-init-local.service seems to be the one blocking for entropy
> here.

So the first thing I'd ask you to investigate is what the heck
cloud-init-local.service is doing, and why it really needs
cryptographic grade random numbers?

> It would be great if this CVE can be fixed somehow without causing boot speed
> to spuike from ~20 seconds to 5 minutes, as that makes the system pretty much
> unusable. I can workaround this by installing haveged, but ideally an 
> in-kernel
> fix would be better. If you need any other info about my setup or if you have
> a patch that I can test, please let me know!

So the question is why is strong random numbers needed by
cloud-init-local, and how much do you trust either haveged and/or
RDRAND (which is what you will be depending upon if you install
rng-tools).  If you believe that Intel and/or the NSA hasn't
backdoored RDRAND[1], or you believe that because Intel processor's
internal cache architecture isn't publically documented, and it's
S complicated that no one can figure it out (which is what you
will be depending upon if you if you choose haveged), be my guest.  I
personally consider the latter to be "security via obscu7rity", but
others have different opinions.

[1] As an aside, read the best paper from the 37th IEEE Symposium on
Security and Privacy and weep:

https://www.computerworld.com/article/3079417/security/researchers-built-devious-undetectable-hardware-level-backdoor-in-computer-chips.html

If it turns out that the security if your VM is critically dependent
on what cloud-init-local.service is doing, and you don't like making
those assumptions, then you may need to ask VMWare ESXi to make
virtio-rng available to guest OS's, and then make Photon OS depend on
a secure RNG available to the host OS.

Best regards,

- Ted

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 17, 2018 at 3:41 PM, Gilad Ben-Yossef  wrote:
> On Thu, May 17, 2018 at 4:35 PM, Geert Uytterhoeven
>  wrote:
>> On Thu, May 17, 2018 at 3:09 PM, Gilad Ben-Yossef  
>> wrote:
>>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>>>  wrote:
 However, even with your clock patch, the signature checking fails for me,
 on both R-Car H3 ES1.0 and ES2.0.
 Does this need changes to the ARM Trusted Firmware, to allow Linux to
 access the public SCEG module?
>>>
>>> Well, this is actually something different. If you look you will
>>> notice that my patch was part of a 3 part patch series,
>>> the first of which disabled this test.
>>
>> Sorry, I had completely forgotten about the first patch from the series.
>> With that applied, it continues:
>>
>> ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version
>> 0x, Driver version 4.0
>> ccree e6601000.crypto: Cache params previous: 0x0777
>> ccree e6601000.crypto: Cache params current: 0x
>> (expect: 0x)
>> alg: No test for cts1(cbc(aes)) (cts1-cbc-aes-ccree)
>> alg: No test for authenc(xcbc(aes),cbc(aes))
>> (authenc-xcbc-aes-cbc-aes-ccree)
>> alg: No test for authenc(xcbc(aes),rfc3686(ctr(aes)))
>> (authenc-xcbc-aes-rfc3686-ctr-aes-ccree)
>> ccree e6601000.crypto: ARM ccree device initialized
>>
>> Is HW version 0x expected?
>
> It's related to the problem with reading the wrong register I've
> mentioned before.

OK.

>>> If you take all the 3 patches, it will work.
>>
>> is there an easy way to test proper operation?
>
> The lines of the form "  alg: No test for cts1(cbc(aes))
> (cts1-cbc-aes-ccree)" indicates
> you have the Crypto API testmgr enable (or rather not disabled would
> be more accurate) so every
> cryptographic algorithm except those specified in these messages was
> tested with test
> vectors from crypto/testmgr.c upon registration. If you don't seen
> failure warnings, it works.

OK.

> You can also check /proc/crypto for all the algorithm with ccree
> listed as their driver and check
> that their test passed.

OK, in that case everything works as expected, on both R-Car H3 ES1.0 and
ES2.0.

Thanks!

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Thu, May 17, 2018 at 4:35 PM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> On Thu, May 17, 2018 at 3:09 PM, Gilad Ben-Yossef  wrote:
>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>>  wrote:
>>> However, even with your clock patch, the signature checking fails for me,
>>> on both R-Car H3 ES1.0 and ES2.0.
>>> Does this need changes to the ARM Trusted Firmware, to allow Linux to
>>> access the public SCEG module?
>>
>> Well, this is actually something different. If you look you will
>> notice that my patch was part of a 3 part patch series,
>> the first of which disabled this test.
>
> Sorry, I had completely forgotten about the first patch from the series.
> With that applied, it continues:
>
> ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version
> 0x, Driver version 4.0
> ccree e6601000.crypto: Cache params previous: 0x0777
> ccree e6601000.crypto: Cache params current: 0x
> (expect: 0x)
> alg: No test for cts1(cbc(aes)) (cts1-cbc-aes-ccree)
> alg: No test for authenc(xcbc(aes),cbc(aes))
> (authenc-xcbc-aes-cbc-aes-ccree)
> alg: No test for authenc(xcbc(aes),rfc3686(ctr(aes)))
> (authenc-xcbc-aes-rfc3686-ctr-aes-ccree)
> ccree e6601000.crypto: ARM ccree device initialized
>
> Is HW version 0x expected?

It's related to the problem with reading the wrong register I've
mentioned before.

>
>> If you take all the 3 patches, it will work.
>
> is there an easy way to test proper operation?

The lines of the form "  alg: No test for cts1(cbc(aes))
(cts1-cbc-aes-ccree)" indicates
you have the Crypto API testmgr enable (or rather not disabled would
be more accurate) so every
cryptographic algorithm except those specified in these messages was
tested with test
vectors from crypto/testmgr.c upon registration. If you don't seen
failure warnings, it works.

You can also check /proc/crypto for all the algorithm with ccree
listed as their driver and check
that their test passed.


> I enabled CONFIG_CRYPT_TEST, and did insmod tcrypt.ko, but I mostly see
> "Failed to load transform" messages.
>

tcrypt.ko is a rather crude developer tool. It has hard coded lists of
test for different algorithms that does
not take into account if some crypto algs are enagled in the build or
not. It's more of a stress test.


Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 17, 2018 at 3:09 PM, Gilad Ben-Yossef  wrote:
> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>  wrote:
>> However, even with your clock patch, the signature checking fails for me,
>> on both R-Car H3 ES1.0 and ES2.0.
>> Does this need changes to the ARM Trusted Firmware, to allow Linux to
>> access the public SCEG module?
>
> Well, this is actually something different. If you look you will
> notice that my patch was part of a 3 part patch series,
> the first of which disabled this test.

Sorry, I had completely forgotten about the first patch from the series.
With that applied, it continues:

ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version
0x, Driver version 4.0
ccree e6601000.crypto: Cache params previous: 0x0777
ccree e6601000.crypto: Cache params current: 0x
(expect: 0x)
alg: No test for cts1(cbc(aes)) (cts1-cbc-aes-ccree)
alg: No test for authenc(xcbc(aes),cbc(aes))
(authenc-xcbc-aes-cbc-aes-ccree)
alg: No test for authenc(xcbc(aes),rfc3686(ctr(aes)))
(authenc-xcbc-aes-rfc3686-ctr-aes-ccree)
ccree e6601000.crypto: ARM ccree device initialized

Is HW version 0x expected?

> If you take all the 3 patches, it will work.

is there an easy way to test proper operation?
I enabled CONFIG_CRYPT_TEST, and did insmod tcrypt.ko, but I mostly see
"Failed to load transform" messages.

Thanks!

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

On Thu, May 17, 2018 at 12:16 PM, Geert Uytterhoeven
 wrote:
> On Thu, May 17, 2018 at 10:01 AM, Gilad Ben-Yossef  
> wrote:
>> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
>>> On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
 On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
 wrote:
 > Add bindings for CryptoCell instance in the SoC.
 >
 > Signed-off-by: Gilad Ben-Yossef 

 Thanks for your patch!

 > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
 > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
 > @@ -528,6 +528,14 @@
 > status = "disabled";
 > };
 >
 > +   arm_cc630p: crypto@e6601000 {
 > +   compatible = "arm,cryptocell-630p-ree";
 > +   interrupts = ;
 > +   #interrupt-cells = <2>;

 I believe the #interrupt-cells property is not needed.

 > +   reg = <0x0 0xe6601000 0 0x1000>;
 > +   clocks = < CPG_MOD 229>;
>
> Missing "power-domains = < R8A7795_PD_ALWAYS_ON>;", as
> the Secure Engine is part of the CPG/MSSR clock domain (see below [*]).

And missing "resets = < 229>;", as the module is tied to the CPG/MSSR
reset controller.

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Thu, May 17, 2018 at 12:04 PM, Simon Horman  wrote:
> On Thu, May 17, 2018 at 11:01:57AM +0300, Gilad Ben-Yossef wrote:
>> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
>> > On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
>> >> Hi Gilad,
>> >>
>> >> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
>> >> wrote:
>> >> > Add bindings for CryptoCell instance in the SoC.
>> >> >
>> >> > Signed-off-by: Gilad Ben-Yossef 
>> >>
>> >> Thanks for your patch!
>> >>
>> >> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> >> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> >> > @@ -528,6 +528,14 @@
>> >> > status = "disabled";
>> >> > };
>> >> >
>> >> > +   arm_cc630p: crypto@e6601000 {
>> >> > +   compatible = "arm,cryptocell-630p-ree";
>> >> > +   interrupts = ;
>> >> > +   #interrupt-cells = <2>;
>> >>
>> >> I believe the #interrupt-cells property is not needed.
>> >>
>> >> > +   reg = <0x0 0xe6601000 0 0x1000>;
>> >> > +   clocks = < CPG_MOD 229>;
>> >> > +   };
>> >>
>> >> The rest looks good, but I cannot verify the register block.
>> >>
>> >> > +
>> >> > i2c3: i2c@e66d {
>> >> > #address-cells = <1>;
>> >> > #size-cells = <0>;
>> >
>> > Thanks, I have applied this after dropping the #interrupt-cells property.
>>
>> Thanks you!
>>
>> Alas, it will not work without the clk patch (the previous one in the
>> series) so they need to be
>> taken or dropped together.
>
> I think its fine if it does not yet work.
> But not if its causes things that previously worked to stop working.

Based on the further discussion with Geert my recommendation is to
drop my patch for now,
take Geert CR clock  patch and I will follow up next week with a v2
that fixes the clock
handing as discussed with Geert.

Many thanks,
Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> On Thu, May 17, 2018 at 10:01 AM, Gilad Ben-Yossef  
> wrote:
>> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
>>> On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
 On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
 wrote:
 > Add bindings for CryptoCell instance in the SoC.
 >
 > Signed-off-by: Gilad Ben-Yossef 

 Thanks for your patch!

 > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
 > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
 > @@ -528,6 +528,14 @@
 > status = "disabled";
 > };
 >
 > +   arm_cc630p: crypto@e6601000 {
 > +   compatible = "arm,cryptocell-630p-ree";
 > +   interrupts = ;
 > +   #interrupt-cells = <2>;

 I believe the #interrupt-cells property is not needed.

 > +   reg = <0x0 0xe6601000 0 0x1000>;
 > +   clocks = < CPG_MOD 229>;
>
> Missing "power-domains = < R8A7795_PD_ALWAYS_ON>;", as
> the Secure Engine is part of the CPG/MSSR clock domain (see below [*]).

Thank you. I didn't get this information from Renesas :-)

>
 > +   };

 The rest looks good, but I cannot verify the register block.

 > +
 > i2c3: i2c@e66d {
 > #address-cells = <1>;
 > #size-cells = <0>;
>>>
>>> Thanks, I have applied this after dropping the #interrupt-cells property.
>>
>> Thanks you!
>>
>> Alas, it will not work without the clk patch (the previous one in the
>> series) so they need to be
>> taken or dropped together.
>
> Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
> does not distinguish between the absence of the clock property, and an
> actual error in getting the clock, and never considers any error a failure
> (incl. -PROBE_DEFER).
>
> As of_clk_get() returns -ENOENT for both a missing clock property and a
> missing clock, you should use (devm_)clk_get() instead, and distinguish
> between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>

Thank you, this is very valuable. I will do as you suggested.


> Hence in the absence of the clock patch, the driver accesses the crypto
> engine while its module clock is turned off, leading to:
>
> ccree e6601000.crypto: Invalid CC signature: SIGNATURE=0x
> != expected=0xDCC63000
>
> You must be lucky, though, usually you get an imprecise external abort
> later, crashing the whole system ;-)
>
> So I think this patch should be dropped for now.
>
> However, even with your clock patch, the signature checking fails for me,
> on both R-Car H3 ES1.0 and ES2.0.
> Does this need changes to the ARM Trusted Firmware, to allow Linux to
> access the public SCEG module?

Well, this is actually something different. If you look you will
notice that my patch was part of a 3 part patch series,
the first of which disabled this test.

If you take all the 3 patches, it will work.

To make things more interesting, I have since sending the patch
learned WHY the test does not work, so disabling
it is not needed - to make a long story short, I was reading the wrong
register that just happens to have the right value
in our FPGA based tests systems but does not in the real silicon
implementations.

But you are right - if the clock is not enabled and you are try to
read from the register the system does freeze.

I will send a fixed v2. based on your patch enabling the CR clock.

>
> [*] More on the subject of clock control:
> At least for Renesas SoCs, where the module is part of a clock domain, and
> can be controlled automatically by Runtime PM, you could drop the explicit
> clock control, and use Runtime PM instead
> (pm_runtime_{enable,get_sync,put,disable}()).  That would allow the driver
> to work on systems with any kind of PM Domains, too.
> Depending on the other platforms that include a CryptoCell and their
> (non)reliance on PM Domains, you may have to keep the explicit clock
> handling, in addition to Runtime PM.
>



> To decrease power consumption, I suggest to move the clock and/or Runtime
> PM handling to the routines that actually use the hardware, instead of
> powering the module in the probe routine.
>

This is very interesting and I will give it a try.

Thanks again!
Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 1/3] crypto: ccree: drop signature register check

[Gilad Ben-Yossef]

Herbert,

On Tue, May 15, 2018 at 3:29 PM, Gilad Ben-Yossef  wrote:
> We were using the content of the signature register as a sanity
> check for the hardware functioning but it turns out not all
> implementers use the same values so the check is giving false
> negative on certain SoCs and so we drop it.
>

Please drop this patch. I have found a better fix and will send a v2 soon.

Thanks,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 17, 2018 at 10:01 AM, Gilad Ben-Yossef  wrote:
> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
>> On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
>>> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
>>> wrote:
>>> > Add bindings for CryptoCell instance in the SoC.
>>> >
>>> > Signed-off-by: Gilad Ben-Yossef 
>>>
>>> Thanks for your patch!
>>>
>>> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>> > @@ -528,6 +528,14 @@
>>> > status = "disabled";
>>> > };
>>> >
>>> > +   arm_cc630p: crypto@e6601000 {
>>> > +   compatible = "arm,cryptocell-630p-ree";
>>> > +   interrupts = ;
>>> > +   #interrupt-cells = <2>;
>>>
>>> I believe the #interrupt-cells property is not needed.
>>>
>>> > +   reg = <0x0 0xe6601000 0 0x1000>;
>>> > +   clocks = < CPG_MOD 229>;

Missing "power-domains = < R8A7795_PD_ALWAYS_ON>;", as
the Secure Engine is part of the CPG/MSSR clock domain (see below [*]).

>>> > +   };
>>>
>>> The rest looks good, but I cannot verify the register block.
>>>
>>> > +
>>> > i2c3: i2c@e66d {
>>> > #address-cells = <1>;
>>> > #size-cells = <0>;
>>
>> Thanks, I have applied this after dropping the #interrupt-cells property.
>
> Thanks you!
>
> Alas, it will not work without the clk patch (the previous one in the
> series) so they need to be
> taken or dropped together.

Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
does not distinguish between the absence of the clock property, and an
actual error in getting the clock, and never considers any error a failure
(incl. -PROBE_DEFER).

As of_clk_get() returns -ENOENT for both a missing clock property and a
missing clock, you should use (devm_)clk_get() instead, and distinguish
between NULL (no clock property) and IS_ERR() (actual failure -> abort).

Hence in the absence of the clock patch, the driver accesses the crypto
engine while its module clock is turned off, leading to:

ccree e6601000.crypto: Invalid CC signature: SIGNATURE=0x
!= expected=0xDCC63000

You must be lucky, though, usually you get an imprecise external abort
later, crashing the whole system ;-)

So I think this patch should be dropped for now.

However, even with your clock patch, the signature checking fails for me,
on both R-Car H3 ES1.0 and ES2.0.
Does this need changes to the ARM Trusted Firmware, to allow Linux to
access the public SCEG module?

[*] More on the subject of clock control:
At least for Renesas SoCs, where the module is part of a clock domain, and
can be controlled automatically by Runtime PM, you could drop the explicit
clock control, and use Runtime PM instead
(pm_runtime_{enable,get_sync,put,disable}()).  That would allow the driver
to work on systems with any kind of PM Domains, too.
Depending on the other platforms that include a CryptoCell and their
(non)reliance on PM Domains, you may have to keep the explicit clock
handling, in addition to Runtime PM.

To decrease power consumption, I suggest to move the clock and/or Runtime
PM handling to the routines that actually use the hardware, instead of
powering the module in the probe routine.

Thanks!

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Simon Horman]

On Thu, May 17, 2018 at 11:01:57AM +0300, Gilad Ben-Yossef wrote:
> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
> > On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
> >> Hi Gilad,
> >>
> >> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
> >> wrote:
> >> > Add bindings for CryptoCell instance in the SoC.
> >> >
> >> > Signed-off-by: Gilad Ben-Yossef 
> >>
> >> Thanks for your patch!
> >>
> >> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> >> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> >> > @@ -528,6 +528,14 @@
> >> > status = "disabled";
> >> > };
> >> >
> >> > +   arm_cc630p: crypto@e6601000 {
> >> > +   compatible = "arm,cryptocell-630p-ree";
> >> > +   interrupts = ;
> >> > +   #interrupt-cells = <2>;
> >>
> >> I believe the #interrupt-cells property is not needed.
> >>
> >> > +   reg = <0x0 0xe6601000 0 0x1000>;
> >> > +   clocks = < CPG_MOD 229>;
> >> > +   };
> >>
> >> The rest looks good, but I cannot verify the register block.
> >>
> >> > +
> >> > i2c3: i2c@e66d {
> >> > #address-cells = <1>;
> >> > #size-cells = <0>;
> >
> > Thanks, I have applied this after dropping the #interrupt-cells property.
> 
> Thanks you!
> 
> Alas, it will not work without the clk patch (the previous one in the
> series) so they need to be
> taken or dropped together.

I think its fine if it does not yet work.
But not if its causes things that previously worked to stop working.

Re: [PATCH 2/3] clk: renesas: r8a7795: Add ccree clock

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 17, 2018 at 10:00 AM, Gilad Ben-Yossef  wrote:
> On Tue, May 15, 2018 at 5:47 PM, Geert Uytterhoeven
>  wrote:
>> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
>> wrote:
>>> This patch adds the clock used by the CryptoCell 630p instance in the SoC.
>>>
>>> Signed-off-by: Gilad Ben-Yossef 
>>
>> Thanks for your patch!
>>
>>> --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
>>> +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
>>> @@ -132,6 +132,7 @@ static struct mssr_mod_clk r8a7795_mod_clks[] 
>>> __initdata = {
>>> DEF_MOD("sys-dmac2", 217,   R8A7795_CLK_S0D3),
>>> DEF_MOD("sys-dmac1", 218,   R8A7795_CLK_S0D3),
>>> DEF_MOD("sys-dmac0", 219,   R8A7795_CLK_S0D3),
>>> +   DEF_MOD("ccree", 229,   R8A7795_CLK_S3D2),
>>
>> I don't know if "ccree" is the proper name for this clock, as there
>> may be multiple
>> instances.
>
> I'd be happy to rename it to anything else. Suggestions?

I believe it should be called "sceg-pub".

>> I also can't verify the parent clock.
>
> I'm afraid I can't really help. This is based on code snippet from
> Renesas. I verified it works but
> I am not an expert on the clock settings :-(

As your driver doesn't care about the clock rate, only about
enabling/disabling the clock, the actual parent doesn't matter much.

After some deeper diving into the datasheet, I believe the correct parent is
the CR clock, which is unfortunately not yet supported by the R-Car H3 clock
driver. I'll send a patch...

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] crypto: reorder paes test lexicographically

[Corentin Labbe]

On Fri, May 11, 2018 at 09:04:06AM +0100, Gilad Ben-Yossef wrote:
> Due to a snafu "paes" testmgr tests were not ordered
> lexicographically, which led to boot time warnings.
> Reorder the tests as needed.
> 
> Fixes: a794d8d ("crypto: ccree - enable support for hardware keys")
> Reported-by: Abdul Haleem 
> Signed-off-by: Gilad Ben-Yossef 

Tested-by: Corentin Labbe 

Thanks

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
> On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
>> Hi Gilad,
>>
>> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
>> wrote:
>> > Add bindings for CryptoCell instance in the SoC.
>> >
>> > Signed-off-by: Gilad Ben-Yossef 
>>
>> Thanks for your patch!
>>
>> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> > @@ -528,6 +528,14 @@
>> > status = "disabled";
>> > };
>> >
>> > +   arm_cc630p: crypto@e6601000 {
>> > +   compatible = "arm,cryptocell-630p-ree";
>> > +   interrupts = ;
>> > +   #interrupt-cells = <2>;
>>
>> I believe the #interrupt-cells property is not needed.
>>
>> > +   reg = <0x0 0xe6601000 0 0x1000>;
>> > +   clocks = < CPG_MOD 229>;
>> > +   };
>>
>> The rest looks good, but I cannot verify the register block.
>>
>> > +
>> > i2c3: i2c@e66d {
>> > #address-cells = <1>;
>> > #size-cells = <0>;
>
> Thanks, I have applied this after dropping the #interrupt-cells property.

Thanks you!

Alas, it will not work without the clk patch (the previous one in the
series) so they need to be
taken or dropped together.

Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 2/3] clk: renesas: r8a7795: Add ccree clock

[Gilad Ben-Yossef]

On Tue, May 15, 2018 at 5:47 PM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  wrote:
>> This patch adds the clock used by the CryptoCell 630p instance in the SoC.
>>
>> Signed-off-by: Gilad Ben-Yossef 
>
> Thanks for your patch!
>
>> --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
>> +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
>> @@ -132,6 +132,7 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata 
>> = {
>> DEF_MOD("sys-dmac2", 217,   R8A7795_CLK_S0D3),
>> DEF_MOD("sys-dmac1", 218,   R8A7795_CLK_S0D3),
>> DEF_MOD("sys-dmac0", 219,   R8A7795_CLK_S0D3),
>> +   DEF_MOD("ccree", 229,   R8A7795_CLK_S3D2),
>
> I don't know if "ccree" is the proper name for this clock, as there
> may be multiple
> instances.

I'd be happy to rename it to anything else. Suggestions?

> I also can't verify the parent clock.

I'm afraid I can't really help. This is based on code snippet from
Renesas. I verified it works but
I am not an expert on the clock settings :-(

>
>> DEF_MOD("cmt3",  300,   R8A7795_CLK_R),
>> DEF_MOD("cmt2",  301,   R8A7795_CLK_R),
>> DEF_MOD("cmt1",  302,   R8A7795_CLK_R),
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 1/5] random: fix crng_ready() test

[Christophe LEROY]

Le 13/04/2018 à 19:00, Theodore Y. Ts'o a écrit :

On Fri, Apr 13, 2018 at 03:05:01PM +0200, Stephan Mueller wrote:


What I would like to point out that more and more folks change to
getrandom(2). As this call will now unblock much later in the boot cycle,
these systems see a significant departure from the current system behavior.

E.g. an sshd using getrandom(2) would be ready shortly after the boot finishes
as of now. Now it can be a matter minutes before it responds. Thus, is such
change in the kernel behavior something for stable?


It will have some change on the kernel behavior, but not as much as
you might think.  That's because in older kernels, we were *already*
blocking until crng_init > 2 --- if the getrandom(2) call happened
while crng_init was in state 0.

Even before this patch series, we didn't wake up a process blocked on
crng_init_wait until crng_init state 2 is reached:

static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
{
...
if (crng == _crng && crng_init < 2) {
invalidate_batched_entropy();
crng_init = 2;
process_random_ready_list();
wake_up_interruptible(_init_wait);
pr_notice("random: crng init done\n");
}
}

This is the reason why there are reports like this: "Boot delayed for
about 90 seconds until 'random: crng init done'"[1]

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1685794


So we have the problem already.  There will be more cases of this
after this patch series is applied, true.  But what we have already is
an inconsistent state where if you call getrandom(2) while the kernel
is in crng_init state 0, you will block until crng_init state 2, but
if you are in crng_init state 1, you will assume the CRNG is fully
initialized.

Given the documentation of how getrandom(2) works what its documented
guarantees are, I think it does justify making its behavior both more
consistent with itself, and more consistent what the security
guarantees we have promised people.

I was a little worried that on VM's this could end up causing things
to block for a long time, but an experiment on a GCE VM shows that
isn't a problem:

[0.00] Linux version 4.16.0-rc3-ext4-9-gf6b302ebca85 (tytso@cwcc) 
(gcc version 7.3.0 (Debian 7.3.0-15)) #16 SMP Thu Apr 12 16:57:17 EDT 2018
[1.282220] random: fast init done
[3.987092] random: crng init done
[4.376787] EXT4-fs (sda1): re-mounted. Opts: (null)

There are some desktops where the "crng_init done" report doesn't
happen until 45-90 seconds into the boot.  I don't think I've seen
reports where it takes _minutes_ however.  Can you give me some
examples of such cases?



On a powerpc embedded board which has an mpc8xx processor running at 
133Mhz, I now get the startup done in more than 7 minutes instead of 30 
seconds. This is due to the webserver blocking on read on /dev/random 
until we get 'random: crng init done':


[0.00] Linux version 4.17.0-rc4-00415-gd2f75d40072d 
(root@localhost) (gcc version 5.4.0 (GCC)) #203 PREEMPT Wed May 16 
16:32:02 CEST 2018
[0.295453] random: get_random_u32 called from 
bucket_table_alloc+0x84/0x1bc with crng_init=0

[1.030472] device: 'random': device_add
[1.031279] device: 'urandom': device_add
[1.420069] device: 'hw_random': device_add
[2.156853] random: fast init done
[  462.007776] random: crng init done

This has become really critical, is there anything that can be done ?

Christophe



- Ted

P.S.  Of course, in a VM environment, if the host supports virtio-rng,
the boot delay problem is completely not an issue.  You just have to
enable virtio-rng in the guest kernel, which I believe is already the
case for most distro kernels.

BTW, for KVM, it's fairly simple to set it the host-side support for
virtio-rng.  Just add to the kvm command-line options:

 -object rng-random,filename=/dev/urandom,id=rng0 \
-device virtio-rng-pci,rng=rng0

Re: [PATCH 1/5] random: fix crng_ready() test

[Srivatsa S. Bhat]

On 4/13/18 10:00 AM, Theodore Y. Ts'o wrote:
> On Fri, Apr 13, 2018 at 03:05:01PM +0200, Stephan Mueller wrote:
>>
>> What I would like to point out that more and more folks change to 
>> getrandom(2). As this call will now unblock much later in the boot cycle, 
>> these systems see a significant departure from the current system behavior.
>>
>> E.g. an sshd using getrandom(2) would be ready shortly after the boot 
>> finishes 
>> as of now. Now it can be a matter minutes before it responds. Thus, is such 
>> change in the kernel behavior something for stable?

[...]

> I was a little worried that on VM's this could end up causing things
> to block for a long time, but an experiment on a GCE VM shows that
> isn't a problem:
> 
> [0.00] Linux version 4.16.0-rc3-ext4-9-gf6b302ebca85 (tytso@cwcc) 
> (gcc version 7.3.0 (Debian 7.3.0-15)) #16 SMP Thu Apr 12 16:57:17 EDT 2018
> [1.282220] random: fast init done
> [3.987092] random: crng init done
> [4.376787] EXT4-fs (sda1): re-mounted. Opts: (null)
> 
> There are some desktops where the "crng_init done" report doesn't
> happen until 45-90 seconds into the boot.  I don't think I've seen
> reports where it takes _minutes_ however.  Can you give me some
> examples of such cases?

On a Photon OS VM running on VMware ESXi, this patch causes a boot speed
regression of 5 minutes :-( [ The VM doesn't have haveged or rng-tools
(rngd) installed. ]

[1.420246] EXT4-fs (sda2): re-mounted. Opts: barrier,noacl,data=ordered
[1.469722] tsc: Refined TSC clocksource calibration: 1900.002 MHz
[1.470707] clocksource: tsc: mask: 0x max_cycles: 
0x36c65c1a9e1, max_idle_ns: 881590695311 ns
[1.474249] clocksource: Switched to clocksource tsc
[1.584427] systemd-journald[216]: Received request to flush runtime journal 
from PID 1
[  346.620718] random: crng init done

Interestingly, the boot delay is exacerbated on VMs with large amounts
of RAM. For example, the delay is not so noticeable (< 30 seconds) on a
VM with 2GB memory, but goes up to 5 minutes on an 8GB VM.

Also, cloud-init-local.service seems to be the one blocking for entropy
here. systemd-analyze critical-chain shows:

The time after the unit is active or started is printed after the "@" character.
The time the unit takes to start is printed after the "+" character.

multi-user.target @6min 1.283s
└─vmtoolsd.service @6min 1.282s
  └─cloud-final.service @6min 366ms +914ms
└─cloud-config.service @5min 59.174s +1.190s
  └─cloud-config.target @5min 59.172s
└─cloud-init.service @5min 47.423s +11.744s
  └─systemd-networkd-wait-online.service @5min 45.999s +1.420s
└─systemd-networkd.service @5min 45.975s +21ms
  └─network-pre.target @5min 45.973s
└─cloud-init-local.service @241ms +5min 45.687s
  └─systemd-remount-fs.service @222ms +13ms
└─systemd-fsck-root.service @193ms +26ms
  └─systemd-journald.socket @188ms
└─-.mount @151ms
  └─system.slice @161ms
└─-.slice @151ms

It would be great if this CVE can be fixed somehow without causing boot speed
to spike from ~20 seconds to 5 minutes, as that makes the system pretty much
unusable. I can workaround this by installing haveged, but ideally an in-kernel
fix would be better. If you need any other info about my setup or if you have
a patch that I can test, please let me know!

Thank you very much!

Regards,
Srivatsa
VMware Photon OS

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Simon Horman]

On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
> Hi Gilad,
> 
> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  wrote:
> > Add bindings for CryptoCell instance in the SoC.
> >
> > Signed-off-by: Gilad Ben-Yossef 
> 
> Thanks for your patch!
> 
> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> > @@ -528,6 +528,14 @@
> > status = "disabled";
> > };
> >
> > +   arm_cc630p: crypto@e6601000 {
> > +   compatible = "arm,cryptocell-630p-ree";
> > +   interrupts = ;
> > +   #interrupt-cells = <2>;
> 
> I believe the #interrupt-cells property is not needed.
> 
> > +   reg = <0x0 0xe6601000 0 0x1000>;
> > +   clocks = < CPG_MOD 229>;
> > +   };
> 
> The rest looks good, but I cannot verify the register block.
> 
> > +
> > i2c3: i2c@e66d {
> > #address-cells = <1>;
> > #size-cells = <0>;

Thanks, I have applied this after dropping the #interrupt-cells property.

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  wrote:
> Add bindings for CryptoCell instance in the SoC.
>
> Signed-off-by: Gilad Ben-Yossef 

Thanks for your patch!

> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> @@ -528,6 +528,14 @@
> status = "disabled";
> };
>
> +   arm_cc630p: crypto@e6601000 {
> +   compatible = "arm,cryptocell-630p-ree";
> +   interrupts = ;
> +   #interrupt-cells = <2>;

I believe the #interrupt-cells property is not needed.

> +   reg = <0x0 0xe6601000 0 0x1000>;
> +   clocks = < CPG_MOD 229>;
> +   };

The rest looks good, but I cannot verify the register block.

> +
> i2c3: i2c@e66d {
> #address-cells = <1>;
> #size-cells = <0>;

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 2/3] clk: renesas: r8a7795: Add ccree clock

[Geert Uytterhoeven]

Hi Gilad,

On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  wrote:
> This patch adds the clock used by the CryptoCell 630p instance in the SoC.
>
> Signed-off-by: Gilad Ben-Yossef 

Thanks for your patch!

> --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
> @@ -132,6 +132,7 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata 
> = {
> DEF_MOD("sys-dmac2", 217,   R8A7795_CLK_S0D3),
> DEF_MOD("sys-dmac1", 218,   R8A7795_CLK_S0D3),
> DEF_MOD("sys-dmac0", 219,   R8A7795_CLK_S0D3),
> +   DEF_MOD("ccree", 229,   R8A7795_CLK_S3D2),

I don't know if "ccree" is the proper name for this clock, as there
may be multiple
instances.
I also can't verify the parent clock.

> DEF_MOD("cmt3",  300,   R8A7795_CLK_R),
> DEF_MOD("cmt2",  301,   R8A7795_CLK_R),
> DEF_MOD("cmt1",  302,   R8A7795_CLK_R),

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] crypto: reorder paes test lexicographically

[Abdul Haleem]

On Fri, 2018-05-11 at 09:04 +0100, Gilad Ben-Yossef wrote:
> Due to a snafu "paes" testmgr tests were not ordered
> lexicographically, which led to boot time warnings.
> Reorder the tests as needed.
> 
> Fixes: a794d8d ("crypto: ccree - enable support for hardware keys")
> Reported-by: Abdul Haleem 
> Signed-off-by: Gilad Ben-Yossef 
> ---
>  crypto/testmgr.c | 44 ++--
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index c31da0f..b1b8ebb 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -3012,13 +3012,6 @@ static const struct alg_test_desc alg_test_descs[] = {
>   }
>   }
>   }, {
> - /* Same as ecb(aes) except the key is stored in
> -  * hardware secure memory which we reference by index
> -  */
> - .alg = "ecb(paes)",
> - .test = alg_test_null,
> - .fips_allowed = 1,
> - }, {
>   .alg = "ecb(khazad)",
>   .test = alg_test_skcipher,
>   .suite = {
> @@ -3028,6 +3021,13 @@ static const struct alg_test_desc alg_test_descs[] = {
>   }
>   }
>   }, {
> + /* Same as ecb(aes) except the key is stored in
> +  * hardware secure memory which we reference by index
> +  */
> + .alg = "ecb(paes)",
> + .test = alg_test_null,
> + .fips_allowed = 1,
> + }, {
>   .alg = "ecb(seed)",
>   .test = alg_test_skcipher,
>   .suite = {
> @@ -3610,21 +3610,6 @@ static const struct alg_test_desc alg_test_descs[] = {
>   }
>   }
>   }, {
> - /* Same as xts(aes) except the key is stored in
> -  * hardware secure memory which we reference by index
> -  */
> - .alg = "xts(paes)",
> - .test = alg_test_null,
> - .fips_allowed = 1,
> - }, {
> - .alg = "xts4096(paes)",
> - .test = alg_test_null,
> - .fips_allowed = 1,
> - }, {
> - .alg = "xts512(paes)",
> - .test = alg_test_null,
> - .fips_allowed = 1,
> - }, {
>   .alg = "xts(camellia)",
>   .test = alg_test_skcipher,
>   .suite = {
> @@ -3643,6 +3628,13 @@ static const struct alg_test_desc alg_test_descs[] = {
>   }
>   }
>   }, {
> + /* Same as xts(aes) except the key is stored in
> +  * hardware secure memory which we reference by index
> +  */
> + .alg = "xts(paes)",
> + .test = alg_test_null,
> + .fips_allowed = 1,
> + }, {
>   .alg = "xts(serpent)",
>   .test = alg_test_skcipher,
>   .suite = {
> @@ -3679,6 +3671,14 @@ static const struct alg_test_desc alg_test_descs[] = {
>   }
>   }
>   }, {
> + .alg = "xts4096(paes)",
> + .test = alg_test_null,
> + .fips_allowed = 1,
> + }, {
> + .alg = "xts512(paes)",
> + .test = alg_test_null,
> + .fips_allowed = 1,
> + }, {
>   .alg = "zlib-deflate",
>   .test = alg_test_comp,
>   .fips_allowed = 1,


Gilad, 

The given patch fixes the boot warnings.

Tested-by: Abdul Haleem 

Thanks for the fix.

-- 
Regard's

Abdul Haleem
IBM Linux Technology Centre

Re: [RFC PATCH 5/5] KEYS: add KPP ecdh parser

[Denis Kenzior]

Hi Tudor,

On 02/28/2018 10:52 AM, Tudor Ambarus wrote:

The ECDH private keys are expected to be encoded with the ecdh
helpers from kernel.

Use the ecdh helpers to check if the key is valid. If valid,
allocate a tfm and set the private key. There is a one-to-one
binding between the private key and the tfm. The tfm is allocated
once and used as many times as needed.

ECDH keys can be loaded like this:
 # echo -n 
020028000200200024d121ebe5cf2d83f6621b6e43843aa38be086c32019da92505303e1c0eab882
 \


This part looks a bit scary.  Isn't this translating directly to 
kpp_secret data structure (in looks like little-endian order) followed 
curve_id, etc.


If the intent is to extend KPP with regular DH, DH + KDF, etc, then we 
might want to invent a proper format here?  I don't think that a Diffie 
Hellman or ECDH Private Key format was ever invented, similar to how 
PKCS8 is used for RSA.


Inventing an ASN.1 syntax would be logical but somewhat painful as D-H 
is frequently used with plain old random numbers and certificates are 
not stored on disk...



 | xxd -r -p | keyctl padd asymmetric private @s

Signed-off-by: Tudor Ambarus 
---
  crypto/asymmetric_keys/Kconfig  |   8 +++
  crypto/asymmetric_keys/Makefile |   5 ++
  crypto/asymmetric_keys/kpp_parser.c | 124 
  include/crypto/asym_kpp_subtype.h   |   2 +
  4 files changed, 139 insertions(+)
  create mode 100644 crypto/asymmetric_keys/kpp_parser.c


Regards,
-Denis

Re: [RFC PATCH 1/5] KEYS: Provide key type operations for kpp ops

[Denis Kenzior]

Hi Tudor,

On 02/28/2018 10:52 AM, Tudor Ambarus wrote:

Provide three new operations in the key_type struct that can be used to
provide access to kpp operations. These will be implemented for the
asymmetric key type in a later patch and may refer to a key retained in
RAM by the kernel or a key retained in crypto hardware.

 int (*asym_kpp_query)(const struct kernel_kpp_params *params,
   struct kernel_kpp_query *res);
 int (*asym_kpp_gen_pubkey)(struct kernel_kpp_params *params,
   void *out);
 int (*asym_kpp_compute_ss)(struct kernel_kpp_params *params,
const void *in, void *out);

Signed-off-by: Tudor Ambarus 
---
  Documentation/security/keys/core.rst | 54 
  include/linux/key-type.h |  7 +
  include/linux/keyctl.h   | 11 
  include/uapi/linux/keyctl.h  |  3 ++
  4 files changed, 75 insertions(+)

diff --git a/Documentation/security/keys/core.rst 
b/Documentation/security/keys/core.rst
index d224fd0..9b69a1f 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -1688,6 +1688,60 @@ The structure has a number of fields, some of which are 
mandatory:
   If successful, 0 will be returned.  If the key doesn't support this,
   EOPNOTSUPP will be returned.
  
+  *  ``int (*asym_kpp_gen_pubkey)(struct kernel_kpp_params *params, void *out);``

+  *  ``int (*asym_kpp_compute_ss)(struct kernel_kpp_params *params, const void 
*in, void *out);``
+
+ These methods are optional. If provided the first allows to generate the
+ public key pair corresponding to the private key. The second method allows
+ to generate a shared secret by  combining the private key and the other
+ party's public key.
+
+ In all cases, the following information is provided in the params block::
+
+   struct kernel_kpp_query {
+   struct key  *key;
+   __u32   in_len; /* Input data size */
+   __u32   out_len;/* Output buffer size */
+   }
+


Probably not a huge deal as most common key sizes are already supported, 
but... is there a way to query supported key sizes?  I think for 
DH_COMPUTE we didn't have this problem as everything was done in 
software.  However, if the intent is to use TPM / other hardware engines 
we might need a way to query supported key sizes.



+ This includes the key to be used and the sizes in bytes of the input and
+ output buffers.
+
+ For a given operation, the in and out buffers are used as follows::
+
+   Operation IDin,in_lenout,out_len
+   === ===  
+   KEYCTL_KPP_GEN_PUBKEY   -Corresponding public key
+   KEYCTL_KPP_COMPUTE_SS   Pair's public keyShared Secret
+
+ If successful, the public key generation and the shared secret computation
+ will return the amount of data written into the output buffer.
+
+  *  ``int (*asym_kpp_query)(const struct kernel_kpp_params *params, struct 
kernel_kpp_query *res);``
+
+ This method is optional. If provided it allows information about the
+ asymmetric KPP (Key Protocol Primitives) key held in the key to be
+ determined.
+
+ The ``params`` block will contain the key to be queried. ``in_len`` and
+ ``out_len`` are unused.
+
+ If successful, the following information is filled in::
+
+   struct kernel_kpp_query {
+   __u32   supported_ops;  /* Which ops are supported */
+   __u32   max_size;   /* Maximum size of the output 
buffer */
+   };
+
+ The supported_ops field will contain a bitmask indicating what operations
+ are supported by the key, including public key generation and shared
+ secret computation. The following constants are defined for this::
+
+   KEYCTL_SUPPORTS_{GEN_PUBKEY, COMPUTE_SS};
+
+ If successful, 0 is returned.  If the key is not an asymmetric kpp key,
+ EOPNOTSUPP is returned.
+
  
  Request-Key Callback Service

  
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index bc9af55..d354b6b 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -19,6 +19,8 @@
  
  struct kernel_pkey_query;

  struct kernel_pkey_params;
+struct kernel_kpp_query;
+struct kernel_kpp_params;
  
  /*

   * key under-construction record
@@ -165,6 +167,11 @@ struct key_type {
   const void *in, void *out);
int (*asym_verify_signature)(struct kernel_pkey_params *params,
 const void *in, const void *in2);
+   int (*asym_kpp_query)(const struct kernel_kpp_params *params,
+ struct kernel_kpp_query *res);
+   int 

Re: [PATCH 2/5] crypto: chtls: wait for memory sendmsg, sendpage

[kbuild test robot]

Hi Atul,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on cryptodev/master]
[also build test WARNING on v4.17-rc5 next-20180514]
[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/Atul-Gupta/build-warnings-cleanup/20180514-213306
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: i386-randconfig-x009-201819 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/crypto/chelsio/chtls/chtls_io.c: In function 'csk_wait_memory':
>> drivers/crypto/chelsio/chtls/chtls_io.c:946:4: warning: this 'if' clause 
>> does not guard... [-Wmisleading-indentation]
   if (noblock)
   ^~
   drivers/crypto/chelsio/chtls/chtls_io.c:948:5: note: ...this statement, but 
the latter is misleadingly indented as if it were guarded by the 'if'
goto do_nonblock;
^~~~

vim +/if +946 drivers/crypto/chelsio/chtls/chtls_io.c

   921  
   922  static int csk_wait_memory(struct chtls_dev *cdev,
   923 struct sock *sk, long *timeo_p)
   924  {
   925  DEFINE_WAIT_FUNC(wait, woken_wake_function);
   926  int sndbuf, err = 0;
   927  long current_timeo;
   928  long vm_wait = 0;
   929  bool noblock;
   930  
   931  current_timeo = *timeo_p;
   932  noblock = (*timeo_p ? false : true);
   933  sndbuf = cdev->max_host_sndbuf;
   934  if (sndbuf > sk->sk_wmem_queued) {
   935  current_timeo = (prandom_u32() % (HZ / 5)) + 2;
   936  vm_wait = (prandom_u32() % (HZ / 5)) + 2;
   937  }
   938  
   939  add_wait_queue(sk_sleep(sk), );
   940  while (1) {
   941  sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk);
   942  
   943  if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
   944  goto do_error;
   945  if (!*timeo_p) {
 > 946  if (noblock)
   947  set_bit(SOCK_NOSPACE, 
>sk_socket->flags);
   948  goto do_nonblock;
   949  }
   950  if (signal_pending(current))
   951  goto do_interrupted;
   952  sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
   953  if (sndbuf > sk->sk_wmem_queued && !vm_wait)
   954  break;
   955  
   956  set_bit(SOCK_NOSPACE, >sk_socket->flags);
   957  sk->sk_write_pending++;
   958  sk_wait_event(sk, _timeo, sk->sk_err ||
   959(sk->sk_shutdown & SEND_SHUTDOWN) ||
   960(sndbuf > sk->sk_wmem_queued && 
!vm_wait), );
   961  sk->sk_write_pending--;
   962  
   963  if (vm_wait) {
   964  vm_wait -= current_timeo;
   965  current_timeo = *timeo_p;
   966  if (current_timeo != MAX_SCHEDULE_TIMEOUT) {
   967  current_timeo -= vm_wait;
   968  if (current_timeo < 0)
   969  current_timeo = 0;
   970  }
   971  vm_wait = 0;
   972  }
   973  *timeo_p = current_timeo;
   974  }
   975  out:
   976  remove_wait_queue(sk_sleep(sk), );
   977  return err;
   978  do_error:
   979  err = -EPIPE;
   980  goto out;
   981  do_nonblock:
   982  err = -EAGAIN;
   983  goto out;
   984  do_interrupted:
   985  err = sock_intr_errno(*timeo_p);
   986  goto out;
   987  }
   988  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: WARNING: kernel stack regs has bad 'bp' value (3)

[Josh Poimboeuf]

On Sat, May 12, 2018 at 12:11:17PM +0200, Ard Biesheuvel wrote:
> On 12 May 2018 at 11:50, Dmitry Vyukov  wrote:
> > On Sat, May 12, 2018 at 11:09 AM, Ard Biesheuvel
> >  wrote:
> >> (+ Arnd)
> >>
> >> On 12 May 2018 at 10:43, Dmitry Vyukov  wrote:
> >>> On Fri, Feb 2, 2018 at 11:18 PM, Eric Biggers  wrote:
>  On Fri, Feb 02, 2018 at 02:57:32PM +0100, Dmitry Vyukov wrote:
> > On Fri, Feb 2, 2018 at 2:48 PM, syzbot
> >  wrote:
> > > Hello,
> > >
> > > syzbot hit the following crash on upstream commit
> > > 7109a04eae81c41ed529da9f3c48c3655ccea741 (Thu Feb 1 17:37:30 2018 
> > > +)
> > > Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide
> > >
> > > So far this crash happened 4 times on net-next, upstream.
> > > C reproducer is attached.
> > > syzkaller reproducer is attached.
> > > Raw console output is attached.
> > > compiler: gcc (GCC) 7.1.1 20170620
> > > .config is attached.
> >
> >
> > From suspicious frames I see salsa20_asm_crypt there, so +crypto 
> > maintainers.
> >
> 
>  Looks like the x86 implementations of Salsa20 (both i586 and x86_64) 
>  need to be
>  updated to not use %ebp/%rbp.
> >>>
> >>> Ard,
> >>>
> >>> This was bisected as introduced by:
> >>>
> >>> commit 83dee2ce1ae791c3dc0c9d4d3a8d42cb109613f6
> >>> Author: Ard Biesheuvel 
> >>> Date:   Fri Jan 19 12:04:34 2018 +
> >>>
> >>> crypto: sha3-generic - rewrite KECCAK transform to help the
> >>> compiler optimize
> >>>
> >>> https://gist.githubusercontent.com/dvyukov/47f93f5a0679170dddf93bc019b42f6d/raw/65beac8ddd30003bbd4e9729236dc8572094abf7/gistfile1.txt
> >>
> >> Ouch.
> >>
> >> I'm not an expert in x86 assembly. Could someone please check the
> >> generated code to see what's going on? The C code changes are not that
> >> intricate, they basically unroll a loop, replacing accesses to
> >> 'array[indirect_index[i]]' with 'array[constant]'.
> >>
> >> As mentioned in the commit log, the speedup is more than significant
> >> for architectures with lots of GPRs so I'd prefer fixing the patch
> >> over reverting it (if there is anything wrong with the code in the
> >> first place)
> >
> > I suspect the problem is with __attribute__((__optimize__("O3"))). It
> > makes compiler use rbp register, which must not be used.
> 
> IIRC, the additional speedup from adding that was significant but not
> huge. Given that we don't use O3 anywhere else, I guess we should just
> remove it.
> 
> Could you please check whether that makes the issue go away?
> 
> If so,
> 
> Acked-by: Ard Biesheuvel 
> 
> for any patch that removes the O3 attribute override from keccakf()

The issue only affects CONFIG_FRAME_POINTER (which is no longer the
default on x86-64), so maybe -O3 could only be enabled for
CONFIG_FRAME_POINTER=n, in which case you'd still get the speedup with
the default ORC unwinder.

-- 
Josh

Re: [PATCH 2/5] crypto: chtls: wait for memory sendmsg, sendpage

[Dan Carpenter]

On Mon, May 14, 2018 at 04:30:56PM +0530, Atul Gupta wrote:
> Reported-by: Gustavo A. R. Silva 
> Signed-off-by: Atul Gupta 

There isn't a commit message for this.  It should say what the user
visible effects of this bug are.  I haven't seen Gustavo's bug report,
but probably copy and pasting that would be good?

> ---
>  drivers/crypto/chelsio/chtls/chtls.h  |  1 +
>  drivers/crypto/chelsio/chtls/chtls_io.c   | 90 
> +--
>  drivers/crypto/chelsio/chtls/chtls_main.c |  1 +
>  3 files changed, 89 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls.h 
> b/drivers/crypto/chelsio/chtls/chtls.h
> index f4b8f1e..778c194 100644
> --- a/drivers/crypto/chelsio/chtls/chtls.h
> +++ b/drivers/crypto/chelsio/chtls/chtls.h
> @@ -149,6 +149,7 @@ struct chtls_dev {
>   struct list_head rcu_node;
>   struct list_head na_node;
>   unsigned int send_page_order;
> + int max_host_sndbuf;
>   struct key_map kmap;
>  };
>  
> diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c 
> b/drivers/crypto/chelsio/chtls/chtls_io.c
> index 5a75be4..a4c7d2d 100644
> --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> @@ -914,6 +914,78 @@ static u16 tls_header_read(struct tls_hdr *thdr, struct 
> iov_iter *from)
>   return (__force u16)cpu_to_be16(thdr->length);
>  }
>  
> +static int csk_mem_free(struct chtls_dev *cdev, struct sock *sk)
> +{
> + return (cdev->max_host_sndbuf - sk->sk_wmem_queued) > 0;

Why not just say:

return (max_host_sndbuf > sk->sk_wmem_queued);

> +}
> +
> +static int csk_wait_memory(struct chtls_dev *cdev,
> +struct sock *sk, long *timeo_p)
> +{
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> + int sndbuf, err = 0;
> + long current_timeo;
> + long vm_wait = 0;
> + bool noblock;
> +
> + current_timeo = *timeo_p;
> + noblock = (*timeo_p ? false : true);
>   sndbuf = cdev->max_host_sndbuf;
> + if (sndbuf > sk->sk_wmem_queued) {

You could use it here:

if (csk_mem_free(cdev, sk)) {


> + current_timeo = (prandom_u32() % (HZ / 5)) + 2;
> + vm_wait = (prandom_u32() % (HZ / 5)) + 2;
> + }
> +
> + add_wait_queue(sk_sleep(sk), );
> + while (1) {
> + sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk);
> +
> + if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
> + goto do_error;
> + if (!*timeo_p) {
> + if (noblock)
> + set_bit(SOCK_NOSPACE, >sk_socket->flags);
> + goto do_nonblock;

There are missing curly braces here.  I feel like these gotos make the
code worse.  They don't remove duplicate lines of code.  They just
spread things out so that you have to jump around to understand this
code.  It's like being a kangaroo.

if (noblock) {
set_bit(SOCK_NOSPACE, >sk_socket->flags);
err = -EAGAIN;
goto remove_queue;
}

I always like picking a descriptive label names instead of "out:"

> + }
> + if (signal_pending(current))
> + goto do_interrupted;
> + sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
> + if (sndbuf > sk->sk_wmem_queued && !vm_wait)
> + break;

if (csk_mem_free(cdev, sk) && !vm_wait)


> +
> + set_bit(SOCK_NOSPACE, >sk_socket->flags);
> + sk->sk_write_pending++;
> + sk_wait_event(sk, _timeo, sk->sk_err ||
> +   (sk->sk_shutdown & SEND_SHUTDOWN) ||
> +   (sndbuf > sk->sk_wmem_queued && !vm_wait), );


  (csk_mem_free(cdev, sk) && !vm_wait), );


> + sk->sk_write_pending--;
> +
> + if (vm_wait) {
> + vm_wait -= current_timeo;
> + current_timeo = *timeo_p;
> + if (current_timeo != MAX_SCHEDULE_TIMEOUT) {
> + current_timeo -= vm_wait;
> + if (current_timeo < 0)
> + current_timeo = 0;
> + }
> + vm_wait = 0;
> + }
> + *timeo_p = current_timeo;
> + }
> +out:
> + remove_wait_queue(sk_sleep(sk), );
> + return err;
> +do_error:
> + err = -EPIPE;
> + goto out;
> +do_nonblock:
> + err = -EAGAIN;
> + goto out;
> +do_interrupted:
> + err = sock_intr_errno(*timeo_p);
> + goto out;
> +}
> +

regards,
dan carpenter

Re: [PATCH 1/2] crypto: vmx - Remove overly verbose printk from AES init routines

[Michael Ellerman]

Herbert Xu  writes:

> On Thu, May 03, 2018 at 10:29:29PM +1000, Michael Ellerman wrote:
>> In the vmx AES init routines we do a printk(KERN_INFO ...) to report
>> the fallback implementation we're using.
>> 
>> However with a slow console this can significantly affect the speed of
>> crypto operations. Using 'cryptsetup benchmark' the removal of the
>> printk() leads to a ~5x speedup for aes-cbc decryption.
>> 
>> So remove them.
>> 
>> Fixes: 8676590a1593 ("crypto: vmx - Adding AES routines for VMX module")
>> Fixes: 8c755ace357c ("crypto: vmx - Adding CBC routines for VMX module")
>> Fixes: 4f7f60d312b3 ("crypto: vmx - Adding CTR routines for VMX module")
>> Fixes: cc333cd68dfa ("crypto: vmx - Adding GHASH routines for VMX module")
>> Cc: sta...@vger.kernel.org # v4.1+
>> Signed-off-by: Michael Ellerman 
>> ---
>>  drivers/crypto/vmx/aes.c | 2 --
>>  drivers/crypto/vmx/aes_cbc.c | 3 ---
>>  drivers/crypto/vmx/aes_ctr.c | 2 --
>>  drivers/crypto/vmx/ghash.c   | 2 --
>>  4 files changed, 9 deletions(-)
>
> Patch applied.  Thanks.

Thanks.

>> If this is the wrong fix please let me know, I'm not a crypto expert.
>> 
>> What we see is 'cryptsetup benchmark' causing thousands of these printks() to
>> happen. The call trace is something like:
>> 
>> [c01e47867a60] [c09cf6b4] p8_aes_cbc_init+0x74/0xf0
>> [c01e47867ae0] [c0551a80] __crypto_alloc_tfm+0x1d0/0x2c0
>> [c01e47867b20] [c055aea4] crypto_skcipher_init_tfm+0x124/0x280
>> [c01e47867b60] [c055138c] crypto_create_tfm+0x9c/0x1a0
>> [c01e47867ba0] [c0552220] crypto_alloc_tfm+0xa0/0x140
>> [c01e47867c00] [c055b168] crypto_alloc_skcipher+0x48/0x70
>> [c01e47867c40] [c057af28] skcipher_bind+0x38/0x50
>> [c01e47867c80] [c057a07c] alg_bind+0xbc/0x220
>> [c01e47867d10] [c0a016a0] __sys_bind+0x90/0x100
>> [c01e47867df0] [c0a01750] sys_bind+0x40/0x60
>> [c01e47867e30] [c000b320] system_call+0x58/0x6c
>> 
>> 
>> Is it normal for init to be called on every system call?
>
> This is the tfm init function, so yes it is called every time
> you allocate a tfm.

OK thanks. So we just shouldn't be printk'ing in there in the non-error
path, good to know.

cheers

Re: [RFC PATCH 0/5] KEYS: add kpp keyctl operations

[Tudor Ambarus]

ping again.

On 04/11/2018 02:08 PM, Tudor Ambarus wrote:

Hi,

There was a long discussion about which interface to chose to export
akcipher and kpp to user-space. This series came as an alternative to
what Stephan proposed for af_alg[1]. I would like some feedback before
diving into tpm.

Best,
ta

[1] https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg27255.html

On 02/28/2018 06:52 PM, Tudor Ambarus wrote:

This series provides keyctl access for kpp operations, including
a query function, a function to generate the public key that is
associated with the private key and a function to compute the
shared secret.

I've added a KPP ecdh parser so that you can load an ECDH private
key into the kernel. The ECDH private keys are expected to be encoded
with the ecdh helpers from kernel. If the private key is valid, the
parser will allocate a tfm and set the private key. There is a
one-to-one binding between the private key and the tfm. The tfm will be
associated with the key for the entire life of the key. The tfm is
allocated once and used as many times as needed.

The kernel patches can be found here also:

https://github.com/ambarus/linux/commits/keys-kpp

The keyutils changes can be found here:

https://github.com/ambarus/keyutils/commits/keys-kpp

These patches are similar to what David Howells proposed for akcipher:

https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg19915.html

I've ported David's patches on top of keys-next and then I've added my
patches on top of them.

Both proposals, David's and mine, lack support for accessing TPM.
Before getting familiar with TPM, please let me know how you feel
about this series.

Tudor Ambarus (5):
   KEYS: Provide key type operations for kpp ops
   KEYS: Provide keyctls to drive the new key type ops for kpp
   KEYS: Provide missing asym kpp subops for new key type ops
   KEYS: add asymmetric kpp subtype
   KEYS: add KPP ecdh parser

  Documentation/security/keys/core.rst | 113 +
  crypto/asymmetric_keys/Kconfig   |  15 +++
  crypto/asymmetric_keys/Makefile  |   6 +
  crypto/asymmetric_keys/asym_kpp.c| 142 +
  crypto/asymmetric_keys/asymmetric_type.c |  77 
  crypto/asymmetric_keys/kpp_parser.c  | 124 +++
  include/crypto/asym_kpp_subtype.h|  14 +++
  include/keys/asymmetric-subtype.h|  12 ++
  include/linux/key-type.h |   7 ++
  include/linux/keyctl.h   |  11 ++
  include/uapi/linux/keyctl.h  |  19 +++
  security/keys/Makefile   |   1 +
  security/keys/compat.c   |  10 ++
  security/keys/internal.h |  28 +
  security/keys/keyctl.c   |  13 ++
  security/keys/keyctl_kpp.c   | 205 
+++

  16 files changed, 797 insertions(+)
  create mode 100644 crypto/asymmetric_keys/asym_kpp.c
  create mode 100644 crypto/asymmetric_keys/kpp_parser.c
  create mode 100644 include/crypto/asym_kpp_subtype.h
  create mode 100644 security/keys/keyctl_kpp.c


--
To unsubscribe from this list: send the line "unsubscribe keyrings" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: WARNING: kernel stack regs has bad 'bp' value (3)

[Ard Biesheuvel]

On 12 May 2018 at 11:50, Dmitry Vyukov  wrote:
> On Sat, May 12, 2018 at 11:09 AM, Ard Biesheuvel
>  wrote:
>> (+ Arnd)
>>
>> On 12 May 2018 at 10:43, Dmitry Vyukov  wrote:
>>> On Fri, Feb 2, 2018 at 11:18 PM, Eric Biggers  wrote:
 On Fri, Feb 02, 2018 at 02:57:32PM +0100, Dmitry Vyukov wrote:
> On Fri, Feb 2, 2018 at 2:48 PM, syzbot
>  wrote:
> > Hello,
> >
> > syzbot hit the following crash on upstream commit
> > 7109a04eae81c41ed529da9f3c48c3655ccea741 (Thu Feb 1 17:37:30 2018 +)
> > Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide
> >
> > So far this crash happened 4 times on net-next, upstream.
> > C reproducer is attached.
> > syzkaller reproducer is attached.
> > Raw console output is attached.
> > compiler: gcc (GCC) 7.1.1 20170620
> > .config is attached.
>
>
> From suspicious frames I see salsa20_asm_crypt there, so +crypto 
> maintainers.
>

 Looks like the x86 implementations of Salsa20 (both i586 and x86_64) need 
 to be
 updated to not use %ebp/%rbp.
>>>
>>> Ard,
>>>
>>> This was bisected as introduced by:
>>>
>>> commit 83dee2ce1ae791c3dc0c9d4d3a8d42cb109613f6
>>> Author: Ard Biesheuvel 
>>> Date:   Fri Jan 19 12:04:34 2018 +
>>>
>>> crypto: sha3-generic - rewrite KECCAK transform to help the
>>> compiler optimize
>>>
>>> https://gist.githubusercontent.com/dvyukov/47f93f5a0679170dddf93bc019b42f6d/raw/65beac8ddd30003bbd4e9729236dc8572094abf7/gistfile1.txt
>>
>> Ouch.
>>
>> I'm not an expert in x86 assembly. Could someone please check the
>> generated code to see what's going on? The C code changes are not that
>> intricate, they basically unroll a loop, replacing accesses to
>> 'array[indirect_index[i]]' with 'array[constant]'.
>>
>> As mentioned in the commit log, the speedup is more than significant
>> for architectures with lots of GPRs so I'd prefer fixing the patch
>> over reverting it (if there is anything wrong with the code in the
>> first place)
>
> I suspect the problem is with __attribute__((__optimize__("O3"))). It
> makes compiler use rbp register, which must not be used.

IIRC, the additional speedup from adding that was significant but not
huge. Given that we don't use O3 anywhere else, I guess we should just
remove it.

Could you please check whether that makes the issue go away?

If so,

Acked-by: Ard Biesheuvel 

for any patch that removes the O3 attribute override from keccakf()

Thanks,
Ard.

Re: WARNING: kernel stack regs has bad 'bp' value (3)

[Dmitry Vyukov]

On Sat, May 12, 2018 at 11:09 AM, Ard Biesheuvel
 wrote:
> (+ Arnd)
>
> On 12 May 2018 at 10:43, Dmitry Vyukov  wrote:
>> On Fri, Feb 2, 2018 at 11:18 PM, Eric Biggers  wrote:
>>> On Fri, Feb 02, 2018 at 02:57:32PM +0100, Dmitry Vyukov wrote:
 On Fri, Feb 2, 2018 at 2:48 PM, syzbot
  wrote:
 > Hello,
 >
 > syzbot hit the following crash on upstream commit
 > 7109a04eae81c41ed529da9f3c48c3655ccea741 (Thu Feb 1 17:37:30 2018 +)
 > Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide
 >
 > So far this crash happened 4 times on net-next, upstream.
 > C reproducer is attached.
 > syzkaller reproducer is attached.
 > Raw console output is attached.
 > compiler: gcc (GCC) 7.1.1 20170620
 > .config is attached.


 From suspicious frames I see salsa20_asm_crypt there, so +crypto 
 maintainers.

>>>
>>> Looks like the x86 implementations of Salsa20 (both i586 and x86_64) need 
>>> to be
>>> updated to not use %ebp/%rbp.
>>
>> Ard,
>>
>> This was bisected as introduced by:
>>
>> commit 83dee2ce1ae791c3dc0c9d4d3a8d42cb109613f6
>> Author: Ard Biesheuvel 
>> Date:   Fri Jan 19 12:04:34 2018 +
>>
>> crypto: sha3-generic - rewrite KECCAK transform to help the
>> compiler optimize
>>
>> https://gist.githubusercontent.com/dvyukov/47f93f5a0679170dddf93bc019b42f6d/raw/65beac8ddd30003bbd4e9729236dc8572094abf7/gistfile1.txt
>
> Ouch.
>
> I'm not an expert in x86 assembly. Could someone please check the
> generated code to see what's going on? The C code changes are not that
> intricate, they basically unroll a loop, replacing accesses to
> 'array[indirect_index[i]]' with 'array[constant]'.
>
> As mentioned in the commit log, the speedup is more than significant
> for architectures with lots of GPRs so I'd prefer fixing the patch
> over reverting it (if there is anything wrong with the code in the
> first place)

I suspect the problem is with __attribute__((__optimize__("O3"))). It
makes compiler use rbp register, which must not be used.

Re: WARNING: kernel stack regs has bad 'bp' value (3)

[Ard Biesheuvel]

(+ Arnd)

On 12 May 2018 at 10:43, Dmitry Vyukov  wrote:
> On Fri, Feb 2, 2018 at 11:18 PM, Eric Biggers  wrote:
>> On Fri, Feb 02, 2018 at 02:57:32PM +0100, Dmitry Vyukov wrote:
>>> On Fri, Feb 2, 2018 at 2:48 PM, syzbot
>>>  wrote:
>>> > Hello,
>>> >
>>> > syzbot hit the following crash on upstream commit
>>> > 7109a04eae81c41ed529da9f3c48c3655ccea741 (Thu Feb 1 17:37:30 2018 +)
>>> > Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide
>>> >
>>> > So far this crash happened 4 times on net-next, upstream.
>>> > C reproducer is attached.
>>> > syzkaller reproducer is attached.
>>> > Raw console output is attached.
>>> > compiler: gcc (GCC) 7.1.1 20170620
>>> > .config is attached.
>>>
>>>
>>> From suspicious frames I see salsa20_asm_crypt there, so +crypto 
>>> maintainers.
>>>
>>
>> Looks like the x86 implementations of Salsa20 (both i586 and x86_64) need to 
>> be
>> updated to not use %ebp/%rbp.
>
> Ard,
>
> This was bisected as introduced by:
>
> commit 83dee2ce1ae791c3dc0c9d4d3a8d42cb109613f6
> Author: Ard Biesheuvel 
> Date:   Fri Jan 19 12:04:34 2018 +
>
> crypto: sha3-generic - rewrite KECCAK transform to help the
> compiler optimize
>
> https://gist.githubusercontent.com/dvyukov/47f93f5a0679170dddf93bc019b42f6d/raw/65beac8ddd30003bbd4e9729236dc8572094abf7/gistfile1.txt

Ouch.

I'm not an expert in x86 assembly. Could someone please check the
generated code to see what's going on? The C code changes are not that
intricate, they basically unroll a loop, replacing accesses to
'array[indirect_index[i]]' with 'array[constant]'.

As mentioned in the commit log, the speedup is more than significant
for architectures with lots of GPRs so I'd prefer fixing the patch
over reverting it (if there is anything wrong with the code in the
first place)

-- 
Ard.

Re: WARNING: kernel stack regs has bad 'bp' value (3)

[Dmitry Vyukov]

On Fri, Feb 2, 2018 at 11:18 PM, Eric Biggers  wrote:
> On Fri, Feb 02, 2018 at 02:57:32PM +0100, Dmitry Vyukov wrote:
>> On Fri, Feb 2, 2018 at 2:48 PM, syzbot
>>  wrote:
>> > Hello,
>> >
>> > syzbot hit the following crash on upstream commit
>> > 7109a04eae81c41ed529da9f3c48c3655ccea741 (Thu Feb 1 17:37:30 2018 +)
>> > Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide
>> >
>> > So far this crash happened 4 times on net-next, upstream.
>> > C reproducer is attached.
>> > syzkaller reproducer is attached.
>> > Raw console output is attached.
>> > compiler: gcc (GCC) 7.1.1 20170620
>> > .config is attached.
>>
>>
>> From suspicious frames I see salsa20_asm_crypt there, so +crypto maintainers.
>>
>
> Looks like the x86 implementations of Salsa20 (both i586 and x86_64) need to 
> be
> updated to not use %ebp/%rbp.

Ard,

This was bisected as introduced by:

commit 83dee2ce1ae791c3dc0c9d4d3a8d42cb109613f6
Author: Ard Biesheuvel 
Date:   Fri Jan 19 12:04:34 2018 +

crypto: sha3-generic - rewrite KECCAK transform to help the
compiler optimize

https://gist.githubusercontent.com/dvyukov/47f93f5a0679170dddf93bc019b42f6d/raw/65beac8ddd30003bbd4e9729236dc8572094abf7/gistfile1.txt

RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support

[Dey, Megha]

>-Original Message-
>From: Herbert Xu [mailto:herb...@gondor.apana.org.au]
>Sent: Thursday, May 10, 2018 9:46 PM
>To: Dey, Megha <megha@intel.com>
>Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org;
>da...@davemloft.net
>Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure
>support
>
>On Fri, May 11, 2018 at 01:24:42AM +, Dey, Megha wrote:
>>
>> Are you suggesting that the SIMD wrapper, will do what is currently being
>done by the ' mcryptd_queue_worker ' function (assuming FPU is not disabled)
>i.e dispatching the job to the inner algorithm?
>>
>> I have got rid of the mcryptd layer( have an inner layer, outer SIMD layer,
>handled the pointers and completions accordingly), but still facing some issues
>after removing the per cpu mcryptd_cpu_queue.
>
>Why don't you post what you've got and we can work it out together?

Hi Herbert,

Sure, I will post an RFC patch. (crypto: Remove mcryptd). 

>
>Thanks,
>--
>Email: Herbert Xu <herb...@gondor.apana.org.au> Home Page:
>http://gondor.apana.org.au/~herbert/
>PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH v2 1/7] crypto: chtls: wait for memory sendmsg, sendpage

[Atul Gupta]

Will do, for further clarity will divide patches into build error fix, bug fix 
and change made for comment received.
Regards

-Original Message-
From: linux-crypto-ow...@vger.kernel.org 
[mailto:linux-crypto-ow...@vger.kernel.org] On Behalf Of Herbert Xu
Sent: Friday, May 11, 2018 9:26 PM
To: Atul Gupta <atul.gu...@chelsio.com>
Cc: linux-crypto@vger.kernel.org; gust...@embeddedor.com
Subject: Re: [PATCH v2 1/7] crypto: chtls: wait for memory sendmsg, sendpage

On Wed, May 02, 2018 at 12:25:33AM +0530, Atul Gupta wrote:
> Reported-by: Gustavo A. R. Silva <gust...@embeddedor.com>
> Signed-off-by: Atul Gupta <atul.gu...@chelsio.com>

These patches really should be sent to netdev as well and I'd like to see some 
acks from there.

Also please add a cover email for the series.

Thanks,
--
Email: Herbert Xu <herb...@gondor.apana.org.au> Home Page: 
http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: vmx - Remove overly verbose printk from AES init routines

[Herbert Xu]

On Thu, May 03, 2018 at 10:29:29PM +1000, Michael Ellerman wrote:
> In the vmx AES init routines we do a printk(KERN_INFO ...) to report
> the fallback implementation we're using.
> 
> However with a slow console this can significantly affect the speed of
> crypto operations. Using 'cryptsetup benchmark' the removal of the
> printk() leads to a ~5x speedup for aes-cbc decryption.
> 
> So remove them.
> 
> Fixes: 8676590a1593 ("crypto: vmx - Adding AES routines for VMX module")
> Fixes: 8c755ace357c ("crypto: vmx - Adding CBC routines for VMX module")
> Fixes: 4f7f60d312b3 ("crypto: vmx - Adding CTR routines for VMX module")
> Fixes: cc333cd68dfa ("crypto: vmx - Adding GHASH routines for VMX module")
> Cc: sta...@vger.kernel.org # v4.1+
> Signed-off-by: Michael Ellerman 
> ---
>  drivers/crypto/vmx/aes.c | 2 --
>  drivers/crypto/vmx/aes_cbc.c | 3 ---
>  drivers/crypto/vmx/aes_ctr.c | 2 --
>  drivers/crypto/vmx/ghash.c   | 2 --
>  4 files changed, 9 deletions(-)

Patch applied.  Thanks.

> If this is the wrong fix please let me know, I'm not a crypto expert.
> 
> What we see is 'cryptsetup benchmark' causing thousands of these printks() to
> happen. The call trace is something like:
> 
> [c01e47867a60] [c09cf6b4] p8_aes_cbc_init+0x74/0xf0
> [c01e47867ae0] [c0551a80] __crypto_alloc_tfm+0x1d0/0x2c0
> [c01e47867b20] [c055aea4] crypto_skcipher_init_tfm+0x124/0x280
> [c01e47867b60] [c055138c] crypto_create_tfm+0x9c/0x1a0
> [c01e47867ba0] [c0552220] crypto_alloc_tfm+0xa0/0x140
> [c01e47867c00] [c055b168] crypto_alloc_skcipher+0x48/0x70
> [c01e47867c40] [c057af28] skcipher_bind+0x38/0x50
> [c01e47867c80] [c057a07c] alg_bind+0xbc/0x220
> [c01e47867d10] [c0a016a0] __sys_bind+0x90/0x100
> [c01e47867df0] [c0a01750] sys_bind+0x40/0x60
> [c01e47867e30] [c000b320] system_call+0x58/0x6c
> 
> 
> Is it normal for init to be called on every system call?

This is the tfm init function, so yes it is called every time
you allocate a tfm.

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

Re: [PATCH resend 00/10] crypto: arm64 - play nice with CONFIG_PREEMPT

[Herbert Xu]

On Mon, Apr 30, 2018 at 06:18:20PM +0200, Ard Biesheuvel wrote:
> Hello Herbert,
> 
> These are the patches that depend on the arm64/assembler.h patches that
> inadvertently got pulled into the cryptodev tree and reverted shortly
> after. Those have now been merged into Linus's tree, and so the
> remaining changes can be applied as well. Please apply.
> 
> Ard Biesheuvel (10):
>   crypto: arm64/sha1-ce - yield NEON after every block of input
>   crypto: arm64/sha2-ce - yield NEON after every block of input
>   crypto: arm64/aes-ccm - yield NEON after every block of input
>   crypto: arm64/aes-blk - yield NEON after every block of input
>   crypto: arm64/aes-bs - yield NEON after every block of input
>   crypto: arm64/aes-ghash - yield NEON after every block of input
>   crypto: arm64/crc32-ce - yield NEON after every block of input
>   crypto: arm64/crct10dif-ce - yield NEON after every block of input
>   crypto: arm64/sha3-ce - yield NEON after every block of input
>   crypto: arm64/sha512-ce - yield NEON after every block of input
> 
>  arch/arm64/crypto/aes-ce-ccm-core.S   | 150 +
>  arch/arm64/crypto/aes-ce.S|  15 +-
>  arch/arm64/crypto/aes-modes.S | 331 
>  arch/arm64/crypto/aes-neonbs-core.S   | 305 ++
>  arch/arm64/crypto/crc32-ce-core.S |  40 ++-
>  arch/arm64/crypto/crct10dif-ce-core.S |  32 +-
>  arch/arm64/crypto/ghash-ce-core.S | 113 +--
>  arch/arm64/crypto/ghash-ce-glue.c |  28 +-
>  arch/arm64/crypto/sha1-ce-core.S  |  42 ++-
>  arch/arm64/crypto/sha2-ce-core.S  |  37 ++-
>  arch/arm64/crypto/sha3-ce-core.S  |  77 +++--
>  arch/arm64/crypto/sha512-ce-core.S|  27 +-
>  12 files changed, 762 insertions(+), 435 deletions(-)

All applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 1/7] crypto: chtls: wait for memory sendmsg, sendpage

[Herbert Xu]

On Wed, May 02, 2018 at 12:25:33AM +0530, Atul Gupta wrote:
> Reported-by: Gustavo A. R. Silva 
> Signed-off-by: Atul Gupta 

These patches really should be sent to netdev as well and I'd
like to see some acks from there.

Also please add a cover email for the series.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [next-20180509][bisected a794d8d] ppc boot warnings at crypto/testmgr.c:3720

[Gilad Ben-Yossef]

On Fri, May 11, 2018 at 9:59 AM, Abdul Haleem
 wrote:
> Greeting's
>
> Today's next kernel on powerpc machine has boot warnings with commit
>
> a794d8d : crypto: ccree - enable support for hardware keys

Adding the crypto list and maintainer as it came in via the crypto tree.

>
> Warning disappears when above commit is reverted.
>
> Machine Type: Power8 PowerVM LPAR
> kernel : 4.17.0-rc4-next-20180509
> config: attached.
> test: kexec boot
>
> trace logs:
> ---
> Initialise system trusted keyrings
> workingset: timestamp_bits=38 max_order=18 bucket_order=0
> zbud: loaded
> pstore: using deflate compression
> WARNING: CPU: 6 PID: 109 at crypto/testmgr.c:3720 alg_test.part.6+0xd4/0x460
> Modules linked in:
> CPU: 6 PID: 109 Comm: cryptomgr_test Not tainted 
> 4.17.0-rc4-next-20180509-autotest-2-g0244ae7 #2
> NIP:  c04a60f4 LR: c04a60e4 CTR: c04a0200
> REGS: c00289acb9d0 TRAP: 0700   Not tainted  
> (4.17.0-rc4-next-20180509-autotest-2-g0244ae7)
> MSR:  80029033   CR: 24000484  XER: 2000
> CFAR: c09b9110 SOFTE: 0
> GPR00: c04a0260 c00289acbc50 c115df00 0001
> GPR04: c0c13e64 000d 0400 
> GPR08:  0001 006b 
> GPR12:  ceca7800 c0128918 c002812a3600
> GPR16:    
> GPR20:  c0c13560 c00288c29880 0400
> GPR24: c00288c29800 000d c0c135a8 c0a1db38
> GPR28: c0c13128 c0c13e60 c0a1c788 0001
> NIP [c04a60f4] alg_test.part.6+0xd4/0x460
> LR [c04a60e4] alg_test.part.6+0xc4/0x460
> Call Trace:
> [c00289acbc50] [c00289acbcc0] 0xc00289acbcc0 (unreliable)
> [c00289acbd90] [c04a0260] cryptomgr_test+0x60/0x80
> [c00289acbdc0] [c0128a68] kthread+0x158/0x1a0
> [c00289acbe30] [c000b628] ret_from_kernel_thread+0x5c/0xb4
> Instruction dump:
> 3b5a56a8 4824 6000 eb9effc8 ebbe 7f83e378 7fa4eb78 48513001
> 6000 7c7f1b78 7d3f00d0 79290fe0 <0b09> 2f9f 419d013c 7fe90034
> ---[ end trace f8ddd7633e720997 ]---
> testmgr: alg_test_descs entries in wrong order: 'ecb(paes)' before 
> 'ecb(khazad)'
>

Oouch! this is is my bad. I've changed the cipher name (from haes to
paes) based on review feedback
but forgot that the cipher test names are alphabetically ordered.

I wonder why I didn't see this myself as I'm 100% sure booted this fix
on 2 different platforms.

Maybe because Khazad was not enabled in my test setup and I guess
there are no other cipher starting in I, L,M,N,O ?

Fix coming up shortly. Sorry about this.

Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support

[Herbert Xu]

On Fri, May 11, 2018 at 01:24:42AM +, Dey, Megha wrote:
> 
> Are you suggesting that the SIMD wrapper, will do what is currently being 
> done by the ' mcryptd_queue_worker ' function (assuming FPU is not disabled) 
> i.e dispatching the job to the inner algorithm?
> 
> I have got rid of the mcryptd layer( have an inner layer, outer SIMD layer, 
> handled the pointers and completions accordingly), but still facing some 
> issues after removing the per cpu mcryptd_cpu_queue.

Why don't you post what you've got and we can work it out together?

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support

[Dey, Megha]

>-Original Message-
>From: Herbert Xu [mailto:herb...@gondor.apana.org.au]
>Sent: Monday, May 7, 2018 2:35 AM
>To: Dey, Megha <megha@intel.com>
>Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org;
>da...@davemloft.net
>Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure
>support
>
>On Tue, May 01, 2018 at 10:39:15PM +, Dey, Megha wrote:
>>
>> crypto/simd.c provides a simd_skcipher_create_compat. I have used the
>> same template to introduce simd_ahash_create_compat which would wrap
>around the inner hash algorithm.
>>
>> Hence we would still register 2 algs, outer and inner.
>
>Right.
>
>> Currently we have outer_alg -> mcryptd alg -> inner_alg
>>
>> Mcryptd is mainly providing the following:
>> 1. Ensuring the lanes(8 in case of AVX2) are full before dispatching to the
>lower inner algorithm. This is obviously why we would expect better
>performance for multi-buffer as opposed to the present single-buffer
>algorithms.
>> 2. If there no new incoming jobs, issue a flush.
>> 3. A glue layer which sends the correct pointers and completions.
>>
>> If we get rid of mcryptd, these functions needs to be done by someone. Since
>all multi-buffer algorithms would require this tasks, where do you suggest 
>these
>helpers live, if not the current mcryptd.c?
>
>That's the issue.  I don't think mcryptd is doing any of these claimed 
>functions
>except for hosting the flush helper which could really live anywhere.
>
>All these functions are actually being carried out in the inner algorithm 
>already.
>
>> I am not sure if you are suggesting that we need to get rid of the mcryptd
>work queue itself. In that case, we would need to execute in the context of the
>job requesting the crypto transformation.
>
>Which is fine as long as you can disable the FPU.  If not the simd wrapper will
>defer the job to kthread context as required.

Hi Herbert,

Are you suggesting that the SIMD wrapper, will do what is currently being done 
by the ' mcryptd_queue_worker ' function (assuming FPU is not disabled) i.e 
dispatching the job to the inner algorithm?

I have got rid of the mcryptd layer( have an inner layer, outer SIMD layer, 
handled the pointers and completions accordingly), but still facing some issues 
after removing the per cpu mcryptd_cpu_queue.
 
>
>Cheers,
>--
>Email: Herbert Xu <herb...@gondor.apana.org.au> Home Page:
>http://gondor.apana.org.au/~herbert/
>PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] async_pq: Remove VLA usage

[Kees Cook]

On Sat, May 5, 2018 at 12:58 AM, Kyle Spiers  wrote:
> In the quest to remove VLAs from the kernel[1], this moves the
> allocation of coefs and blocks from the stack to being kmalloc()ed.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Kyle Spiers 

Reviewed-by: Kees Cook 

Is this something that should go via Vinod, Dan, or direct through
Herbert's crypto tree?

Thanks!

-Kees

> ---
> Forgot to add slab.h
> ---
>  crypto/async_tx/async_pq.c  | 18 ++
>  crypto/async_tx/raid6test.c |  9 -
>  2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/crypto/async_tx/async_pq.c b/crypto/async_tx/async_pq.c
> index 56bd612927ab..af1912313a23 100644
> --- a/crypto/async_tx/async_pq.c
> +++ b/crypto/async_tx/async_pq.c
> @@ -194,9 +194,9 @@ async_gen_syndrome(struct page **blocks, unsigned int 
> offset, int disks,
> (src_cnt <= dma_maxpq(device, 0) ||
>  dma_maxpq(device, DMA_PREP_CONTINUE) > 0) &&
> is_dma_pq_aligned(device, offset, 0, len)) {
> -   struct dma_async_tx_descriptor *tx;
> +   struct dma_async_tx_descriptor *tx = NULL;
> enum dma_ctrl_flags dma_flags = 0;
> -   unsigned char coefs[src_cnt];
> +   unsigned char *coefs;
> int i, j;
>
> /* run the p+q asynchronously */
> @@ -207,6 +207,9 @@ async_gen_syndrome(struct page **blocks, unsigned int 
> offset, int disks,
>  * sources and update the coefficients accordingly
>  */
> unmap->len = len;
> +   coefs = kmalloc_array(src_cnt, sizeof(*coefs), GFP_KERNEL);
> +   if (!coefs)
> +   goto out;
> for (i = 0, j = 0; i < src_cnt; i++) {
> if (blocks[i] == NULL)
> continue;
> @@ -240,7 +243,9 @@ async_gen_syndrome(struct page **blocks, unsigned int 
> offset, int disks,
> }
>
> tx = do_async_gen_syndrome(chan, coefs, j, unmap, dma_flags, 
> submit);
> +out:
> dmaengine_unmap_put(unmap);
> +   kfree(coefs);
> return tx;
> }
>
> @@ -298,8 +303,8 @@ async_syndrome_val(struct page **blocks, unsigned int 
> offset, int disks,
>  {
> struct dma_chan *chan = pq_val_chan(submit, blocks, disks, len);
> struct dma_device *device = chan ? chan->device : NULL;
> -   struct dma_async_tx_descriptor *tx;
> -   unsigned char coefs[disks-2];
> +   struct dma_async_tx_descriptor *tx = NULL;
> +   unsigned char *coefs = NULL;
> enum dma_ctrl_flags dma_flags = submit->cb_fn ? DMA_PREP_INTERRUPT : 
> 0;
> struct dmaengine_unmap_data *unmap = NULL;
>
> @@ -318,6 +323,9 @@ async_syndrome_val(struct page **blocks, unsigned int 
> offset, int disks,
>  __func__, disks, len);
>
> unmap->len = len;
> +   coefs = kmalloc_array(disks - 2, sizeof(*coefs), GFP_KERNEL);
> +   if (!coefs)
> +   goto out;
> for (i = 0; i < disks-2; i++)
> if (likely(blocks[i])) {
> unmap->addr[j] = dma_map_page(dev, blocks[i],
> @@ -423,6 +431,8 @@ async_syndrome_val(struct page **blocks, unsigned int 
> offset, int disks,
> async_tx_sync_epilog(submit);
> tx = NULL;
> }
> +out:
> +   kfree(coefs);
> dmaengine_unmap_put(unmap);
>
> return tx;
> diff --git a/crypto/async_tx/raid6test.c b/crypto/async_tx/raid6test.c
> index dad95f45b88f..4237a5ae8f42 100644
> --- a/crypto/async_tx/raid6test.c
> +++ b/crypto/async_tx/raid6test.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #undef pr
>  #define pr(fmt, args...) pr_info("raid6test: " fmt, ##args)
> @@ -81,11 +82,16 @@ static void raid6_dual_recov(int disks, size_t bytes, int 
> faila, int failb, stru
> init_async_submit(, 0, NULL, NULL, NULL, 
> addr_conv);
> tx = async_gen_syndrome(ptrs, 0, disks, bytes, 
> );
> } else {
> -   struct page *blocks[disks];
> +   struct page **blocks;
> struct page *dest;
> int count = 0;
> int i;
>
> +   blocks = kmalloc_array(disks, sizeof(*blocks),
> +   GFP_KERNEL);
> +   if (!blocks)
> +   return;
> +
> /* data+Q failure.  Reconstruct data from P,
>  * then rebuild syndrome
>  */
> @@ -101,6 +107,7 @@ static void raid6_dual_recov(int disks, size_t bytes, int 
> faila, int failb, stru
>
>

Re: DMA map buffer allocated in ahash_request_ctx

[Harsh Jain]

On Wed, May 9, 2018 at 8:37 PM, Herbert Xu  wrote:
> On Wed, May 09, 2018 at 04:13:12PM +0300, Gilad Ben-Yossef wrote:
>> On Wed, May 9, 2018 at 3:12 PM, Harsh Jain  wrote:
>> > Hi Herbert,
>> >
>> > Can we use buffer defined in ahash request context(private space for
>> > each request) to DMA map it to H/W.?
>>
>> Not Herbert but... the ccree driver is doing just and Herbert
>> specifically indicated ahash request contexts are not allowed to be
>> allocated from stack (I asked) so it should be OK.
>
> That's right.  For async algorithms the request must come from
> kmalloc memory.

Thanks Herbert/Gilad.

It means, It's perfectly fine to have array of required(For DMA)
memory in request context structure and DMA map it later on.

>
> Cheers,
> --
> Email: Herbert Xu 
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: DMA map buffer allocated in ahash_request_ctx

[Herbert Xu]

On Wed, May 09, 2018 at 04:13:12PM +0300, Gilad Ben-Yossef wrote:
> On Wed, May 9, 2018 at 3:12 PM, Harsh Jain  wrote:
> > Hi Herbert,
> >
> > Can we use buffer defined in ahash request context(private space for
> > each request) to DMA map it to H/W.?
> 
> Not Herbert but... the ccree driver is doing just and Herbert
> specifically indicated ahash request contexts are not allowed to be
> allocated from stack (I asked) so it should be OK.

That's right.  For async algorithms the request must come from
kmalloc memory.

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

Re: DMA map buffer allocated in ahash_request_ctx

[Gilad Ben-Yossef]

On Wed, May 9, 2018 at 3:12 PM, Harsh Jain  wrote:
> Hi Herbert,
>
> Can we use buffer defined in ahash request context(private space for
> each request) to DMA map it to H/W.?

Not Herbert but... the ccree driver is doing just and Herbert
specifically indicated ahash request contexts are not allowed to be
allocated from stack (I asked) so it should be OK.

Note that you will need to map and unmap at every Crypto API invocation.
You can't map on init because you are not guaranteed to have a final()
or export()

Hope it helps,
Gilad
-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH v15 net-next 00/12] Chelsio Inline TLS

[Atul Gupta]

On 4/1/2018 6:27 PM, Boris Pismenny wrote:
> Hi,
>
> On 4/1/2018 6:37 AM, David Miller wrote:
>> From: Atul Gupta <atul.gu...@chelsio.com>
>> Date: Sat, 31 Mar 2018 21:41:51 +0530
>>
>>> Series for Chelsio Inline TLS driver (chtls)
>>
>> Series applied, thank you.
>>
>
> Sorry for being late to the party, could you please help answer a few 
> questions to help me understand better.
Going over these points and addressing some of them in follow-up patches:
>
> 1. What happens if someone attempts to set a TCP socket option for a socket 
> whose TCP stack resides in the TCP offload engine(TOE)? Do you emulate all 
> Linux socket options? What about IP socket options?
HW offloaded options are handled while rest shall be redirected to host.
>
> If I follow the code correctly, then the original TCP/IP setsockopt is 
> called. But, it doesn't change any of the parameters of the TCP/IP offload 
> engine in hardware.
>
> 2. I can't find where is the TLS record sequence number pushed to hardware. 
> Is that on purpose?
seq-nos is pushed to HW in cpl_tx_tls_sfo->scmd1
>
> FYI, ignoring this parameter might cause a record sequence number reuse which 
> breaks the integrity of the AES-GCM TLS ciphersuite.
>
> 3. How does a TOE handle Tx only or Rx only?
Driver does not differentiate/isolate the tx and rx path for Inline Processing
>
> 4. What happens when there is a routing change that redirects traffic to a 
> different netdev? Is there a software fallback?
The case we think is handling the next hop change, is there any other case?
>
> 5. The TLS socket option is set in the middle of a TCP connection. What 
> happens to the existing TCP connection and the data that is currently queued 
> in the TCP write queue?
I believe this behave same as SW. If by TLS options you mean re-keying then 
outstanding data on Tx is flushed before new key takes effect. For Rx user 
should be careful else it will result in MAC error.

Thanks
Atul
>
> Thanks,
> Boris.
>

Re: [PATCH 00/18] Fix some build warnings/errors with Sphinx

[Luis R. Rodriguez]

On Tue, May 08, 2018 at 10:13:42AM -0600, Jonathan Corbet wrote:
> On Mon,  7 May 2018 06:35:36 -0300
> Mauro Carvalho Chehab  wrote:
> 
> > I decided to give a try with Sphinx last stable version
> > (1.17.4), and noticed several issues. The worse one was
> > with the networking book: a non-standard footnote there
> > with [*] instead of a number causes it to break PDF building.
> > 
> > So, I took some spare time to address some warnings all over
> > the tree, and moved a few text documents to a book. 
> 
> OK, I've applied the ones that seem to make sense for me to take now.
> There's comments on the firmware one, 

I'll fold in the fixes for the firmware API which do apply to my queue.

  Luis

Re: [PATCH 00/18] Fix some build warnings/errors with Sphinx

[Jonathan Corbet]

On Mon,  7 May 2018 06:35:36 -0300
Mauro Carvalho Chehab  wrote:

> I decided to give a try with Sphinx last stable version
> (1.17.4), and noticed several issues. The worse one was
> with the networking book: a non-standard footnote there
> with [*] instead of a number causes it to break PDF building.
> 
> So, I took some spare time to address some warnings all over
> the tree, and moved a few text documents to a book. 

OK, I've applied the ones that seem to make sense for me to take now.
There's comments on the firmware one, and I'd rather have Tejun's OK for
the cgroup one.  The code-comment changes should probably go via the
usual maintainers.

> I with
> I had more time to move the other ones and to solve other
> warnings.

You and me both - but each step helps!

Thanks,

jon

Re: [PATCH 03/18] docs: */index.rst: Add newer documents to their respective index.rst

[Jonathan Corbet]

On Mon,  7 May 2018 06:35:39 -0300
Mauro Carvalho Chehab  wrote:

> A number of new docs were added, but they're currently not on
> the index.rst from the session they're supposed to be, causing
> Sphinx warnings.
> 
> Add them.
> 
> Signed-off-by: Mauro Carvalho Chehab 

I've applied this one, thanks.

jon

Re: [PATCH 03/18] docs: */index.rst: Add newer documents to their respective index.rst

[Greg Kroah-Hartman]

On Mon, May 07, 2018 at 06:35:39AM -0300, Mauro Carvalho Chehab wrote:
> A number of new docs were added, but they're currently not on
> the index.rst from the session they're supposed to be, causing
> Sphinx warnings.
> 
> Add them.
> 
> Signed-off-by: Mauro Carvalho Chehab 

Reviewed-by: Greg Kroah-Hartman 

Re: [PATCH v2 0/5] crypto: Speck support

[Eric Biggers]

Hi Samuel,

On Thu, Apr 26, 2018 at 03:05:44AM +0100, Samuel Neves wrote:
> On Wed, Apr 25, 2018 at 8:49 PM, Eric Biggers  wrote:
> > I agree that my explanation should have been better, and should have 
> > considered
> > more crypto algorithms.  The main difficulty is that we have extreme 
> > performance
> > requirements -- it needs to be 50 MB/s at the very least on even low-end ARM
> > devices like smartwatches.  And even with the NEON-accelerated Speck128-XTS
> > performance exceeding that after much optimization, we've been getting a 
> > lot of
> > pushback as people want closer to 100 MB/s.
> >
> 
> I couldn't find any NEON-capable ARMv7 chip below 800 MHz, so this
> would put the performance upper bound around 15 cycles per byte, with
> the comfortable number being ~7. That's indeed tough, though not
> impossible.
> 
> >
> > That's why I also included Speck64-XTS in the patches, since it was
> > straightforward to include, and some devices may really need that last 
> > 20-30% of
> > performance for encryption to be feasible at all.  (And when the choice is
> > between unencrypted and a 64-bit block cipher, used in a context where the
> > weakest points in the cryptosystem are actually elsewhere such as the user's
> > low-entropy PIN and the flash storage doing wear-leveling, I'd certainly 
> > take
> > the 64-bit block cipher.)  So far we haven't had to use Speck64 though, and 
> > if
> > that continues to be the case I'd be fine with Speck64 being removed, 
> > leaving
> > just Speck128.
> >
> 
> I would very much prefer that to be the case. As many of us know,
> "it's better than nothing" has been often used to justify other bad
> choices, like RC4, that end up preventing better ones from being
> adopted. At a time where we're trying to get rid of 64-bit ciphers in
> TLS, where data volumes per session are comparatively low, it would be
> unfortunate if the opposite starts happening on encryption at rest.
> 
> >
> > Note that in practice, to have any chance at meeting the performance 
> > requirement
> > the cipher needed to be NEON accelerated.  That made benchmarking really 
> > hard
> > and time-consuming, since to definitely know how an algorithm performs it 
> > can
> > take upwards of a week to implement a NEON version.  It needs to be very 
> > well
> > optimized too, to compare the algorithms fairly -- e.g. with Speck I got a 
> > 20%
> > performance improvement on some CPUs just by changing the NEON instructions 
> > used
> > to implement the 8-bit rotates, an optimization that is not possible with
> > ciphers that don't use rotate amounts that are multiples of 8.  (This was an
> > intentional design choice by the Speck designers; they do know what they're
> > doing, actually.)
> >
> > Thus, we had to be pretty aggressive about dropping algorithms from
> > consideration if there were preliminary indications that they wouldn't 
> > perform
> > well, or had too little cryptanalysis, or had other issues such as an 
> > unclear
> > patent situation.  Threefish for example I did test the C implementation at
> > https://github.com/wernerd/Skein3Fish, but on ARM32 it was over 4 times 
> > slower
> > than my NEON implementation of Speck128/256-XTS.  And I did not see a clear 
> > way
> > that it could be improved over 4x with NEON, if at all, so I did not take 
> > the
> > long time it would have taken to write an optimized NEON implementation to
> > benchmark it properly.  Perhaps that was a mistake.  But, time is not 
> > unlimited.
> >
> 
> In my limited experience with NEON and 64-bit ARX, there's usually a
> ~2x speedup solely from NEON's native 64-bit operations on ARMv7-A.
> The extra speedup from encrypting 2 block in parallel is then
> somewhere between 1x and 2x, depending on various details. Getting
> near 4x might be feasible, but it is indeed time-consuming to get
> there.
> 
> >
> > As for the wide-block mode using ChaCha20 and Poly1305, you'd have to ask 
> > Paul
> > Crowley to explain it properly, but briefly it's actually a pseudorandom
> > permutation over an arbitrarily-sized message.  So with dm-crypt for 
> > example, it
> > would operate on a whole 512-byte sector, and if any bit of the 512-byte
> > plaintext is changed, then every bit in the 512-byte ciphertext would change
> > with 50% probability.  To make this possible, the construction uses a 
> > polynomial
> > evalution in GF(2^130-5) as a universal hash function, similar to the 
> > Poly1305
> > mode.
> >
> 
> Oh, OK, that sounds like something resembling Naor-Reingold or its
> relatives. That would work, but with 3 or 4 passes I guess it wouldn't
> be very fast.
> 
> >
> > Using ChaCha20's underlying 512-bit permutation to build a tweakable block
> > cipher is an interesting idea.  But maybe in my crypto-naivety, it is not
> > obvious to me how to do so.  Do you have references to any relevant papers?
> > Remember that we strongly prefer a published cipher to a custom one -- even 
> > if
> 

Re: IV generation

[Gilad Ben-Yossef]

On Mon, May 7, 2018 at 2:29 PM, Stephan Mueller  wrote:
> Am Montag, 7. Mai 2018, 13:19:47 CEST schrieb Gilad Ben-Yossef:
>
> Hi Gilad,
>
>> ah... so if I have hardware that can implement say, seqiv, I can
>> register "seqiv(rfc4106(gcm(aes)))" and, assuming priorities are
>> right, it will be used?
>
> That is the question I cannot fully answer. Seqiv is a template and thus not
> subjet to prios by itself. So, you hardware however could register the full
> seqiv(rfc) cipher. I am not fully sure that such registered cipher is then
> picked up by the IPSec stack.
>
> Look into net/xfrm/xfrm_algos.c -- there you see the individual cipher names
> and the IV generator added separately. What I have not traced yet is whether
> the code assembles the IV generator name and the cipher name before making the
> call to crypto_alloc_aead.
>
> What I can say for sure is that the kernel crypto API knows of the
> seqiv(rfc...) cipher name and generates the IV for your (the invocation field
> that is).

I see.

I think the code does the assembly in esp4.c esp_init_aead() and
esp_init_authenc()

So it should all Just Work(TM).

Many thanks for the clarification.

Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: IV generation

[Stephan Mueller]

Am Montag, 7. Mai 2018, 13:19:47 CEST schrieb Gilad Ben-Yossef:

Hi Gilad,

> ah... so if I have hardware that can implement say, seqiv, I can
> register "seqiv(rfc4106(gcm(aes)))" and, assuming priorities are
> right, it will be used?

That is the question I cannot fully answer. Seqiv is a template and thus not 
subjet to prios by itself. So, you hardware however could register the full 
seqiv(rfc) cipher. I am not fully sure that such registered cipher is then 
picked up by the IPSec stack.

Look into net/xfrm/xfrm_algos.c -- there you see the individual cipher names 
and the IV generator added separately. What I have not traced yet is whether 
the code assembles the IV generator name and the cipher name before making the 
call to crypto_alloc_aead.

What I can say for sure is that the kernel crypto API knows of the 
seqiv(rfc...) cipher name and generates the IV for your (the invocation field 
that is).


Ciao
Stephan

Re: IV generation

[Gilad Ben-Yossef]

On Mon, May 7, 2018 at 2:02 PM, Stephan Mueller  wrote:
> Am Montag, 7. Mai 2018, 08:26:08 CEST schrieb Gilad Ben-Yossef:
>
> Hi Gilad,
>
>> Hi,
>>
>> A quick question: am I correct in my understanding that there is now
>> no automatic IV generation support for either skcipher nor aead?
>> And if I'm wrong, can someone point to an example of a driver that
>> implements either, as all the ones I see are the deprecated ablkcipher
>> interface.
>>
>> BTW, I'm perfectly fine with not having one, I just want to understand
>> I am not missing something...
>
> The automated IV generation is implemented with the generators such as seqiv
> or chainiv.
>
> For example, AES-GCM as used for IPSec compliant with RFC4106 generates the IV
> (the invocation field part of the IV) with the seqiv. This is handled by the
> IPSec stack to initialize the cipher of, say, seqiv(rfc4106(gcm(aes))).
>
> The CTR mode uses the chainiv implementation to manage the IV.

ah... so if I have hardware that can implement say, seqiv, I can
register "seqiv(rfc4106(gcm(aes)))" and, assuming priorities are
right, it will be used?

Thanks,
Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru