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

[PATCH v2 2/5] crypto: ccree: better clock handling

[Gilad Ben-Yossef]

Use managed clock handling, differentiate between no clock (possibly OK)
and clock init failure (never OK) and correctly handle clock detection
being deferred.

Suggested-by: Geert Uytterhoeven 
Signed-off-by: Gilad Ben-Yossef 
---
 drivers/crypto/ccree/cc_driver.c | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 6f93ce7..b266657 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -190,6 +190,7 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
u64 dma_mask;
const struct cc_hw_data *hw_rev;
const struct of_device_id *dev_id;
+   struct clk *clk;
int rc = 0;
 
new_drvdata = devm_kzalloc(dev, sizeof(*new_drvdata), GFP_KERNEL);
@@ -219,7 +220,24 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
platform_set_drvdata(plat_dev, new_drvdata);
new_drvdata->plat_dev = plat_dev;
 
-   new_drvdata->clk = of_clk_get(np, 0);
+   clk = devm_clk_get(dev, NULL);
+   if (IS_ERR(clk))
+   switch (PTR_ERR(clk)) {
+   /* Clock is optional so this might be fine */
+   case -ENOENT:
+   break;
+
+   /* Clock not available, let's try again soon */
+   case -EPROBE_DEFER:
+   return -EPROBE_DEFER;
+
+   default:
+   dev_err(dev, "Error getting clock: %ld\n",
+   PTR_ERR(clk));
+   return PTR_ERR(clk);
+   }
+   new_drvdata->clk = clk;
+
new_drvdata->coherent = of_dma_is_coherent(np);
 
/* Get device resources */
-- 
2.7.4

[PATCH v2 3/5] crypto: ccree: silence debug prints

[Gilad Ben-Yossef]

The cache parameter register configuration was being too verbose.
Use dev_dbg() to only provide the information if needed.

Signed-off-by: Gilad Ben-Yossef 
---
 drivers/crypto/ccree/cc_driver.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index b266657..1e40e76 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -168,14 +168,14 @@ int init_cc_regs(struct cc_drvdata *drvdata, bool 
is_probe)
val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
 
if (is_probe)
-   dev_info(dev, "Cache params previous: 0x%08X\n", val);
+   dev_dbg(dev, "Cache params previous: 0x%08X\n", val);
 
cc_iowrite(drvdata, CC_REG(AXIM_CACHE_PARAMS), cache_params);
val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
 
if (is_probe)
-   dev_info(dev, "Cache params current: 0x%08X (expect: 0x%08X)\n",
-val, cache_params);
+   dev_dbg(dev, "Cache params current: 0x%08X (expect: 0x%08X)\n",
+   val, cache_params);
 
return 0;
 }
-- 
2.7.4

[PATCH v2 5/5] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

Add bindings for CryptoCell instance in the SoC.

Signed-off-by: Gilad Ben-Yossef 
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi 
b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index d842940..3ac75db 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -528,6 +528,15 @@
status = "disabled";
};
 
+   arm_cc630p: crypto@e6601000 {
+   compatible = "arm,cryptocell-630p-ree";
+   interrupts = ;
+   reg = <0x0 0xe6601000 0 0x1000>;
+   clocks = < CPG_MOD 229>;
+   resets = < 229>;
+   power-domains = < R8A7795_PD_ALWAYS_ON>;
+   };
+
i2c3: i2c@e66d {
#address-cells = <1>;
#size-cells = <0>;
-- 
2.7.4

[PATCH v2 4/5] clk: renesas: r8a7795: add ccree clock bindings

[Gilad Ben-Yossef]

This patch adds the clock used by the CryptoCell 630p instance in the SoC.

Signed-off-by: Gilad Ben-Yossef 
---

This patch depends upon the "clk: renesas: r8a7795: Add CR clock" patch
from Geert Uytterhoeven.

 drivers/clk/renesas/r8a7795-cpg-mssr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c 
b/drivers/clk/renesas/r8a7795-cpg-mssr.c
index e5b1865..a85dd50 100644
--- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
@@ -133,6 +133,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("sceg-pub",  229,   R8A7795_CLK_CR),
DEF_MOD("cmt3",  300,   R8A7795_CLK_R),
DEF_MOD("cmt2",  301,   R8A7795_CLK_R),
DEF_MOD("cmt1",  302,   R8A7795_CLK_R),
-- 
2.7.4

[PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling

[Gilad Ben-Yossef]

The patch set enables the use of CryptoCell found in some Renesas R-Car
Salvator-X boards and fixes some driver issues uncovered that prevented
to work properly.

Changes from v1:
- Properly fix the bug that caused us to read a bad signature register
  rather than dropping the check
- Proper DT fields as indicated by Geert Uytterhoeven.
- Better clock enabling as suggested by Geert Uytterhoeven.

Note! the last two patches in the set depend on the
"clk: renesas: r8a7795: Add CR clock" patch from Geert Uytterhoeven.

Gilad Ben-Yossef (5):
  crypto: ccree: correct host regs offset
  crypto: ccree: better clock handling
  crypto: ccree: silence debug prints
  clk: renesas: r8a7795: add ccree clock bindings
  arm64: dts: renesas: r8a7795: add ccree binding

 arch/arm64/boot/dts/renesas/r8a7795.dtsi |  9 +
 drivers/clk/renesas/r8a7795-cpg-mssr.c   |  1 +
 drivers/crypto/ccree/cc_debugfs.c|  7 +--
 drivers/crypto/ccree/cc_driver.c | 34 ++--
 drivers/crypto/ccree/cc_driver.h |  2 ++
 drivers/crypto/ccree/cc_host_regs.h  |  6 --
 6 files changed, 49 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] crypto: ccree: correct host regs offset

[Gilad Ben-Yossef]

The product signature and HW revision register have different offset on the
older HW revisions.
This fixes the problem of the driver failing sanity check on silicon
despite working on the FPGA emulation systems.

Fixes: 27b3b22dd98c ("crypto: ccree - add support for older HW revs")
Cc: sta...@vger.kernel.org
Signed-off-by: Gilad Ben-Yossef 
---
 drivers/crypto/ccree/cc_debugfs.c   | 7 +--
 drivers/crypto/ccree/cc_driver.c| 8 ++--
 drivers/crypto/ccree/cc_driver.h| 2 ++
 drivers/crypto/ccree/cc_host_regs.h | 6 --
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/ccree/cc_debugfs.c 
b/drivers/crypto/ccree/cc_debugfs.c
index 08f8db4..5ca184e 100644
--- a/drivers/crypto/ccree/cc_debugfs.c
+++ b/drivers/crypto/ccree/cc_debugfs.c
@@ -26,7 +26,8 @@ struct cc_debugfs_ctx {
 static struct dentry *cc_debugfs_dir;
 
 static struct debugfs_reg32 debug_regs[] = {
-   CC_DEBUG_REG(HOST_SIGNATURE),
+   { .name = "SIGNATURE" }, /* Must be 0th */
+   { .name = "VERSION" }, /* Must be 1st */
CC_DEBUG_REG(HOST_IRR),
CC_DEBUG_REG(HOST_POWER_DOWN_EN),
CC_DEBUG_REG(AXIM_MON_ERR),
@@ -34,7 +35,6 @@ static struct debugfs_reg32 debug_regs[] = {
CC_DEBUG_REG(HOST_IMR),
CC_DEBUG_REG(AXIM_CFG),
CC_DEBUG_REG(AXIM_CACHE_PARAMS),
-   CC_DEBUG_REG(HOST_VERSION),
CC_DEBUG_REG(GPR_HOST),
CC_DEBUG_REG(AXIM_MON_COMP),
 };
@@ -58,6 +58,9 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
struct debugfs_regset32 *regset;
struct dentry *file;
 
+   debug_regs[0].offset = drvdata->sig_offset;
+   debug_regs[1].offset = drvdata->ver_offset;
+
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 89ce013..6f93ce7 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -207,9 +207,13 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
if (hw_rev->rev >= CC_HW_REV_712) {
new_drvdata->hash_len_sz = HASH_LEN_SIZE_712;
new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP);
+   new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_712);
+   new_drvdata->ver_offset = CC_REG(HOST_VERSION_712);
} else {
new_drvdata->hash_len_sz = HASH_LEN_SIZE_630;
new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP8);
+   new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_630);
+   new_drvdata->ver_offset = CC_REG(HOST_VERSION_630);
}
 
platform_set_drvdata(plat_dev, new_drvdata);
@@ -276,7 +280,7 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
}
 
/* Verify correct mapping */
-   signature_val = cc_ioread(new_drvdata, CC_REG(HOST_SIGNATURE));
+   signature_val = cc_ioread(new_drvdata, new_drvdata->sig_offset);
if (signature_val != hw_rev->sig) {
dev_err(dev, "Invalid CC signature: SIGNATURE=0x%08X != 
expected=0x%08X\n",
signature_val, hw_rev->sig);
@@ -287,7 +291,7 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
 
/* Display HW versions */
dev_info(dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver 
version %s\n",
-hw_rev->name, cc_ioread(new_drvdata, CC_REG(HOST_VERSION)),
+hw_rev->name, cc_ioread(new_drvdata, new_drvdata->ver_offset),
 DRV_MODULE_VERSION);
 
rc = init_cc_regs(new_drvdata, true);
diff --git a/drivers/crypto/ccree/cc_driver.h b/drivers/crypto/ccree/cc_driver.h
index 2048fde..95f82b2 100644
--- a/drivers/crypto/ccree/cc_driver.h
+++ b/drivers/crypto/ccree/cc_driver.h
@@ -129,6 +129,8 @@ struct cc_drvdata {
enum cc_hw_rev hw_rev;
u32 hash_len_sz;
u32 axim_mon_offset;
+   u32 sig_offset;
+   u32 ver_offset;
 };
 
 struct cc_crypto_alg {
diff --git a/drivers/crypto/ccree/cc_host_regs.h 
b/drivers/crypto/ccree/cc_host_regs.h
index f510018..616b2e1 100644
--- a/drivers/crypto/ccree/cc_host_regs.h
+++ b/drivers/crypto/ccree/cc_host_regs.h
@@ -45,7 +45,8 @@
 #define CC_HOST_ICR_DSCRPTR_WATERMARK_QUEUE0_CLEAR_BIT_SIZE0x1UL
 #define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SHIFT  0x17UL
 #define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SIZE   0x1UL
-#define CC_HOST_SIGNATURE_REG_OFFSET   0xA24UL
+#define CC_HOST_SIGNATURE_712_REG_OFFSET   0xA24UL
+#define CC_HOST_SIGNATURE_630_REG_OFFSET   0xAC8UL
 #define CC_HOST_SIGNATURE_VALUE_BIT_SHIFT  0x0UL
 #define CC_HOST_SIGNATURE_VALUE_BIT_SIZE   0x20UL
 #define CC_HOST_BOOT_REG_OFFSET0xA28UL
@@ -105,7 +106,8 @@
 #define CC_HOST_BOOT_ONLY_ENCRYPT_LOCAL_BIT_SIZE   0x1UL
 #define CC_HOST_BOOT_AES_EXISTS_LOCAL_BIT_SHIFT0x1EUL
 #define CC_HOST_BOOT_AES_EXISTS_LOCAL_BIT_SIZE 0x1UL

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

[PATCH 0/3] crypto:chelsio: Fixes and cleanup

[Harsh Jain]

It includes Fixes and cleanup .

Harsh Jain (3):
  crypto:chelsio:Return -ENOSPC for transient busy indication.
  crypt:chelsio:Send IV as Immediate for cipher algo
  crypto:chelsio: Remove separate buffer used for DMA map B0 block in
CCM

 drivers/crypto/chelsio/chcr_algo.c   | 303 +++
 drivers/crypto/chelsio/chcr_algo.h   |   3 +-
 drivers/crypto/chelsio/chcr_core.h   |   2 +-
 drivers/crypto/chelsio/chcr_crypto.h |  15 +-
 4 files changed, 140 insertions(+), 183 deletions(-)

-- 
2.1.4

[PATCH 1/3] crypto:chelsio:Return -ENOSPC for transient busy indication.

[Harsh Jain]

Change the return type based on following patch
https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg28552.html

Signed-off-by: Harsh Jain 
---
 drivers/crypto/chelsio/chcr_algo.c | 56 ++
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/drivers/crypto/chelsio/chcr_algo.c 
b/drivers/crypto/chelsio/chcr_algo.c
index 59fe6631e..db15a9f 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -688,6 +688,7 @@ static int chcr_cipher_fallback(struct crypto_skcipher 
*cipher,
int err;
 
SKCIPHER_REQUEST_ON_STACK(subreq, cipher);
+
skcipher_request_set_tfm(subreq, cipher);
skcipher_request_set_callback(subreq, flags, NULL, NULL);
skcipher_request_set_crypt(subreq, src, dst,
@@ -1113,14 +1114,6 @@ static int chcr_handle_cipher_resp(struct 
ablkcipher_request *req,
goto complete;
}
 
-   if (unlikely(cxgb4_is_crypto_q_full(u_ctx->lldi.ports[0],
-   c_ctx(tfm)->tx_qidx))) {
-   if (!(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) {
-   err = -EBUSY;
-   goto unmap;
-   }
-
-   }
if (!reqctx->imm) {
bytes = chcr_sg_ent_in_wr(reqctx->srcsg, reqctx->dstsg, 1,
  CIP_SPACE_LEFT(ablkctx->enckey_len),
@@ -1293,13 +1286,14 @@ static int chcr_aes_encrypt(struct ablkcipher_request 
*req)
 {
struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
struct sk_buff *skb = NULL;
-   int err;
+   int err, isfull = 0;
struct uld_ctx *u_ctx = ULD_CTX(c_ctx(tfm));
 
if (unlikely(cxgb4_is_crypto_q_full(u_ctx->lldi.ports[0],
c_ctx(tfm)->tx_qidx))) {
+   isfull = 1;
if (!(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG))
-   return -EBUSY;
+   return -ENOSPC;
}
 
err = process_cipher(req, u_ctx->lldi.rxq_ids[c_ctx(tfm)->rx_qidx],
@@ -1309,7 +1303,7 @@ static int chcr_aes_encrypt(struct ablkcipher_request 
*req)
skb->dev = u_ctx->lldi.ports[0];
set_wr_txq(skb, CPL_PRIORITY_DATA, c_ctx(tfm)->tx_qidx);
chcr_send_wr(skb);
-   return -EINPROGRESS;
+   return isfull ? -EBUSY : -EINPROGRESS;
 }
 
 static int chcr_aes_decrypt(struct ablkcipher_request *req)
@@ -1317,12 +1311,13 @@ static int chcr_aes_decrypt(struct ablkcipher_request 
*req)
struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
struct uld_ctx *u_ctx = ULD_CTX(c_ctx(tfm));
struct sk_buff *skb = NULL;
-   int err;
+   int err, isfull = 0;
 
if (unlikely(cxgb4_is_crypto_q_full(u_ctx->lldi.ports[0],
c_ctx(tfm)->tx_qidx))) {
+   isfull = 1;
if (!(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG))
-   return -EBUSY;
+   return -ENOSPC;
}
 
 err = process_cipher(req, u_ctx->lldi.rxq_ids[c_ctx(tfm)->rx_qidx],
@@ -1332,7 +1327,7 @@ static int chcr_aes_decrypt(struct ablkcipher_request 
*req)
skb->dev = u_ctx->lldi.ports[0];
set_wr_txq(skb, CPL_PRIORITY_DATA, c_ctx(tfm)->tx_qidx);
chcr_send_wr(skb);
-   return -EINPROGRESS;
+   return isfull ? -EBUSY : -EINPROGRESS;
 }
 
 static int chcr_device_init(struct chcr_context *ctx)
@@ -1574,14 +1569,15 @@ static int chcr_ahash_update(struct ahash_request *req)
u8 remainder = 0, bs;
unsigned int nbytes = req->nbytes;
struct hash_wr_param params;
-   int error;
+   int error, isfull = 0;
 
bs = crypto_tfm_alg_blocksize(crypto_ahash_tfm(rtfm));
u_ctx = ULD_CTX(h_ctx(rtfm));
if (unlikely(cxgb4_is_crypto_q_full(u_ctx->lldi.ports[0],
h_ctx(rtfm)->tx_qidx))) {
+   isfull = 1;
if (!(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG))
-   return -EBUSY;
+   return -ENOSPC;
}
 
if (nbytes + req_ctx->reqlen >= bs) {
@@ -1633,7 +1629,7 @@ static int chcr_ahash_update(struct ahash_request *req)
set_wr_txq(skb, CPL_PRIORITY_DATA, h_ctx(rtfm)->tx_qidx);
chcr_send_wr(skb);
 
-   return -EINPROGRESS;
+   return isfull ? -EBUSY : -EINPROGRESS;
 unmap:
chcr_hash_dma_unmap(_ctx->lldi.pdev->dev, req);
return error;
@@ -1710,15 +1706,16 @@ static int chcr_ahash_finup(struct ahash_request *req)
struct sk_buff *skb;
struct hash_wr_param params;
u8  bs;
-   int error;
+   int error, isfull = 0;
 
bs = crypto_tfm_alg_blocksize(crypto_ahash_tfm(rtfm));
u_ctx = ULD_CTX(h_ctx(rtfm));
 
if (unlikely(cxgb4_is_crypto_q_full(u_ctx->lldi.ports[0],

[PATCH 2/3] crypt:chelsio:Send IV as Immediate for cipher algo

[Harsh Jain]

Send IV in WR as immediate instead of dma mapped entry for cipher.

Signed-off-by: Harsh Jain 
---
 drivers/crypto/chelsio/chcr_algo.c   | 49 +++-
 drivers/crypto/chelsio/chcr_algo.h   |  3 +--
 drivers/crypto/chelsio/chcr_core.h   |  2 +-
 drivers/crypto/chelsio/chcr_crypto.h |  3 +--
 4 files changed, 17 insertions(+), 40 deletions(-)

diff --git a/drivers/crypto/chelsio/chcr_algo.c 
b/drivers/crypto/chelsio/chcr_algo.c
index db15a9f..b2bfeb2 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -638,7 +638,6 @@ static int chcr_sg_ent_in_wr(struct scatterlist *src,
src = sg_next(src);
srcskip = 0;
}
-
if (sg_dma_len(dst) == dstskip) {
dst = sg_next(dst);
dstskip = 0;
@@ -761,13 +760,13 @@ static struct sk_buff *create_cipher_wr(struct 
cipher_wr_param *wrparam)
 
nents = sg_nents_xlen(reqctx->dstsg,  wrparam->bytes, CHCR_DST_SG_SIZE,
  reqctx->dst_ofst);
-   dst_size = get_space_for_phys_dsgl(nents + 1);
+   dst_size = get_space_for_phys_dsgl(nents);
kctx_len = roundup(ablkctx->enckey_len, 16);
transhdr_len = CIPHER_TRANSHDR_SIZE(kctx_len, dst_size);
nents = sg_nents_xlen(reqctx->srcsg, wrparam->bytes,
  CHCR_SRC_SG_SIZE, reqctx->src_ofst);
-   temp = reqctx->imm ? roundup(IV + wrparam->req->nbytes, 16) :
-(sgl_len(nents + MIN_CIPHER_SG) * 8);
+   temp = reqctx->imm ? roundup(wrparam->bytes, 16) :
+(sgl_len(nents) * 8);
transhdr_len += temp;
transhdr_len = roundup(transhdr_len, 16);
skb = alloc_skb(SGE_MAX_WR_LEN, flags);
@@ -789,7 +788,7 @@ static struct sk_buff *create_cipher_wr(struct 
cipher_wr_param *wrparam)
 ablkctx->ciph_mode,
 0, 0, IV >> 1);
chcr_req->sec_cpl.ivgen_hdrlen = FILL_SEC_CPL_IVGEN_HDRLEN(0, 0, 0,
- 0, 0, dst_size);
+ 0, 1, dst_size);
 
chcr_req->key_ctx.ctx_hdr = ablkctx->key_ctx_hdr;
if ((reqctx->op == CHCR_DECRYPT_OP) &&
@@ -819,8 +818,8 @@ static struct sk_buff *create_cipher_wr(struct 
cipher_wr_param *wrparam)
chcr_add_cipher_dst_ent(wrparam->req, phys_cpl, wrparam, wrparam->qid);
 
atomic_inc(>chcr_stats.cipher_rqst);
-   temp = sizeof(struct cpl_rx_phys_dsgl) + dst_size + kctx_len
-   +(reqctx->imm ? (IV + wrparam->bytes) : 0);
+   temp = sizeof(struct cpl_rx_phys_dsgl) + dst_size + kctx_len + IV
+   + (reqctx->imm ? (wrparam->bytes) : 0);
create_wreq(c_ctx(tfm), chcr_req, &(wrparam->req->base), reqctx->imm, 0,
transhdr_len, temp,
ablkctx->ciph_mode == CHCR_SCMD_CIPHER_MODE_AES_CBC);
@@ -1023,7 +1022,7 @@ static int chcr_update_tweak(struct ablkcipher_request 
*req, u8 *iv,
ret = crypto_cipher_setkey(cipher, key, keylen);
if (ret)
goto out;
-   /*H/W sends the encrypted IV in dsgl when AADIVDROP bit is 0*/
+   crypto_cipher_encrypt_one(cipher, iv, iv);
for (i = 0; i < round8; i++)
gf128mul_x8_ble((le128 *)iv, (le128 *)iv);
 
@@ -1115,7 +1114,7 @@ static int chcr_handle_cipher_resp(struct 
ablkcipher_request *req,
}
 
if (!reqctx->imm) {
-   bytes = chcr_sg_ent_in_wr(reqctx->srcsg, reqctx->dstsg, 1,
+   bytes = chcr_sg_ent_in_wr(reqctx->srcsg, reqctx->dstsg, 0,
  CIP_SPACE_LEFT(ablkctx->enckey_len),
  reqctx->src_ofst, reqctx->dst_ofst);
if ((bytes + reqctx->processed) >= req->nbytes)
@@ -1126,11 +1125,7 @@ static int chcr_handle_cipher_resp(struct 
ablkcipher_request *req,
/*CTR mode counter overfloa*/
bytes  = req->nbytes - reqctx->processed;
}
-   dma_sync_single_for_cpu(_CTX(c_ctx(tfm))->lldi.pdev->dev,
-   reqctx->iv_dma, IV, DMA_BIDIRECTIONAL);
err = chcr_update_cipher_iv(req, fw6_pld, reqctx->iv);
-   dma_sync_single_for_device(_CTX(c_ctx(tfm))->lldi.pdev->dev,
-  reqctx->iv_dma, IV, DMA_BIDIRECTIONAL);
if (err)
goto unmap;
 
@@ -1205,7 +1200,6 @@ static int process_cipher(struct ablkcipher_request *req,
 
dnents = sg_nents_xlen(req->dst, req->nbytes,
   CHCR_DST_SG_SIZE, 0);
-   dnents += 1; // IV
phys_dsgl = get_space_for_phys_dsgl(dnents);
kctx_len = roundup(ablkctx->enckey_len, 16);
transhdr_len = 

[PATCH 3/3] crypto:chelsio: Remove separate buffer used for DMA map B0 block in CCM

[Harsh Jain]

Extends memory required for IV to include B0 Block and DMA map in
single operation.

Signed-off-by: Harsh Jain 
---
 drivers/crypto/chelsio/chcr_algo.c   | 198 ---
 drivers/crypto/chelsio/chcr_crypto.h |  12 +--
 2 files changed, 97 insertions(+), 113 deletions(-)

diff --git a/drivers/crypto/chelsio/chcr_algo.c 
b/drivers/crypto/chelsio/chcr_algo.c
index b2bfeb2..b916c4e 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -203,13 +203,8 @@ static inline void chcr_handle_aead_resp(struct 
aead_request *req,
 int err)
 {
struct chcr_aead_reqctx *reqctx = aead_request_ctx(req);
-   struct crypto_aead *tfm = crypto_aead_reqtfm(req);
-   struct uld_ctx *u_ctx = ULD_CTX(a_ctx(tfm));
 
-   chcr_aead_dma_unmap(_ctx->lldi.pdev->dev, req, reqctx->op);
-   if (reqctx->b0_dma)
-   dma_unmap_single(_ctx->lldi.pdev->dev, reqctx->b0_dma,
-reqctx->b0_len, DMA_BIDIRECTIONAL);
+   chcr_aead_common_exit(req);
if (reqctx->verify == VERIFY_SW) {
chcr_verify_tag(req, input, );
reqctx->verify = VERIFY_HW;
@@ -2178,22 +2173,35 @@ static void chcr_hmac_cra_exit(struct crypto_tfm *tfm)
}
 }
 
-static int chcr_aead_common_init(struct aead_request *req,
-unsigned short op_type)
+inline void chcr_aead_common_exit(struct aead_request *req)
+{
+   struct chcr_aead_reqctx  *reqctx = aead_request_ctx(req);
+   struct crypto_aead *tfm = crypto_aead_reqtfm(req);
+   struct uld_ctx *u_ctx = ULD_CTX(a_ctx(tfm));
+
+   chcr_aead_dma_unmap(_ctx->lldi.pdev->dev, req, reqctx->op);
+}
+
+static int chcr_aead_common_init(struct aead_request *req)
 {
struct crypto_aead *tfm = crypto_aead_reqtfm(req);
struct chcr_aead_ctx *aeadctx = AEAD_CTX(a_ctx(tfm));
struct chcr_aead_reqctx  *reqctx = aead_request_ctx(req);
-   int error = -EINVAL;
unsigned int authsize = crypto_aead_authsize(tfm);
+   int error = -EINVAL;
 
/* validate key size */
if (aeadctx->enckey_len == 0)
goto err;
-   if (op_type && req->cryptlen < authsize)
+   if (reqctx->op && req->cryptlen < authsize)
goto err;
+   if (reqctx->b0_len)
+   reqctx->scratch_pad = reqctx->iv + IV;
+   else
+   reqctx->scratch_pad = NULL;
+
error = chcr_aead_dma_map(_CTX(a_ctx(tfm))->lldi.pdev->dev, req,
- op_type);
+ reqctx->op);
if (error) {
error = -ENOMEM;
goto err;
@@ -2230,7 +2238,7 @@ static int chcr_aead_fallback(struct aead_request *req, 
unsigned short op_type)
aead_request_set_tfm(subreq, aeadctx->sw_cipher);
aead_request_set_callback(subreq, req->base.flags,
  req->base.complete, req->base.data);
-aead_request_set_crypt(subreq, req->src, req->dst, req->cryptlen,
+   aead_request_set_crypt(subreq, req->src, req->dst, req->cryptlen,
 req->iv);
 aead_request_set_ad(subreq, req->assoclen);
return op_type ? crypto_aead_decrypt(subreq) :
@@ -2239,8 +2247,7 @@ static int chcr_aead_fallback(struct aead_request *req, 
unsigned short op_type)
 
 static struct sk_buff *create_authenc_wr(struct aead_request *req,
 unsigned short qid,
-int size,
-unsigned short op_type)
+int size)
 {
struct crypto_aead *tfm = crypto_aead_reqtfm(req);
struct chcr_aead_ctx *aeadctx = AEAD_CTX(a_ctx(tfm));
@@ -2264,18 +2271,20 @@ static struct sk_buff *create_authenc_wr(struct 
aead_request *req,
if (req->cryptlen == 0)
return NULL;
 
-   reqctx->b0_dma = 0;
+   reqctx->b0_len = 0;
+   error = chcr_aead_common_init(req);
+   if (error)
+   return ERR_PTR(error);
+
if (subtype == CRYPTO_ALG_SUB_TYPE_CBC_NULL ||
-   subtype == CRYPTO_ALG_SUB_TYPE_CTR_NULL) {
+   subtype == CRYPTO_ALG_SUB_TYPE_CTR_NULL) {
null = 1;
assoclen = 0;
+   reqctx->aad_nents = 0;
}
-   error = chcr_aead_common_init(req, op_type);
-   if (error)
-   return ERR_PTR(error);
dnents = sg_nents_xlen(req->dst, assoclen, CHCR_DST_SG_SIZE, 0);
dnents += sg_nents_xlen(req->dst, req->cryptlen +
-   (op_type ? -authsize : authsize), CHCR_DST_SG_SIZE,
+   (reqctx->op ? -authsize : authsize), CHCR_DST_SG_SIZE,
req->assoclen);
dnents += MIN_AUTH_SG; // For IV
 
@@ -2292,11 +2301,10 @@ static struct sk_buff *create_authenc_wr(struct 
aead_request *req,
  

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