Re: Alg errors with Intel QAT Card

[Neil Horman]

On Tue, Jun 13, 2017 at 01:32:55PM -0700, Raj Ammanur wrote:
> Hi Neil & Salvatore,
> 
> thanks for the replies. The soft reboot hasn't helped. I am trying a previous
> kernel version that works with a similar card that we installed in
> another server
> and that works fine. Will keep you posted.
> 
> Neil: have you found fix/workaround for the firmware errors or you just using
> a soft reboot? by that, you just mean unload and load the kernel modules or
> a system reboot. I have tried both.
> 
its the latter that works around the issue.  I belive a QAT firmware update will
be forthcomming to fix the issue

Neil

> thanks
> --Raj
> 
> On Tue, Jun 13, 2017 at 5:30 AM, Neil Horman <nhor...@tuxdriver.com> wrote:
> > On Mon, Jun 12, 2017 at 03:52:07PM +, Benedetto, Salvatore wrote:
> >> Hi Raj,
> >>
> >> I've compiled and tested kernel 4.12.0-rc4 and I can't reproduce your 
> >> issue.
> >> Are you seeing any of this with a previous kernel version? If not, git 
> >> bisect might
> >> help us finding the root-cause.
> >> Have you tried with another platform/hw?
> >>
> >> Regards,
> >> Salvatore
> >>
> > Try a soft reboot and see if the error clears up.  This looks a bit 
> > reminscient
> > of some firmware errors we've been chasing down
> > Neil
> >
> >> > -Original Message-
> >> > From: linux-crypto-ow...@vger.kernel.org [mailto:linux-crypto-
> >> > ow...@vger.kernel.org] On Behalf Of Raj Ammanur
> >> > Sent: Friday, June 9, 2017 7:37 PM
> >> > To: Linux Crypto Mailing List <linux-crypto@vger.kernel.org>
> >> > Subject: Alg errors with Intel QAT Card
> >> >
> >> > Hi
> >> >
> >> > I am seeing the below errors after  installing an Intel QAT card
> >> > and loading the upstreamed qat_dh895xcc and intel_qat modules.
> >> >
> >> > Have others seen similar errors and know if this is a known issue
> >> > and a fix exists or know whats going on ? This is with 4.12.0-rc4+
> >> > version of the kernel.
> >> >
> >> > Any help is sincerely appreciated.
> >> >
> >> > thanks
> >> > --Raj
> >> >
> >> >
> >> > [3.639046] dh895xcc :00:0b.0: qat_dev0 started 12 acceleration 
> >> > engines
> >> > [4.168887] alg: skcipher-ddst: Test 5 failed (invalid result) on
> >> > encryption for qat_aes_cbc
> >> > [4.217866] alg: skcipher-ddst: Chunk test 1 failed on encryption
> >> > at page 0 for qat_aes_ctr
> >> > [4.282042] alg: skcipher: Test 4 failed (invalid result) on
> >> > encryption for qat_aes_xts
> >> > [4.395210] alg: akcipher: test 1 failed for qat-rsa, err=-22
> >> > [root@dhcp-swlab-681 ~]# dmesg | grep -i alg
> >> > [1.499336] alg: No test for pkcs1pad(rsa,sha256)
> >> > (pkcs1pad(rsa-generic,sha256))
> >> > [2.562511] SELinux:  Class alg_socket not defined in policy.
> >> > [4.168887] alg: skcipher-ddst: Test 5 failed (invalid result) on
> >> > encryption for qat_aes_cbc
> >> > [4.217866] alg: skcipher-ddst: Chunk test 1 failed on encryption
> >> > at page 0 for qat_aes_ctr
> >> > [4.282042] alg: skcipher: Test 4 failed (invalid result) on
> >> > encryption for qat_aes_xts
> >> > [4.367682] alg: akcipher: encrypt test failed. Invalid output
> >> > [4.395210] alg: akcipher: test 1 failed for qat-rsa, err=-22
> >> > [4.431827] alg: dh: generate public key test failed. Invalid output
> >> > [4.431829] alg: dh: test failed on vector 1, err=-22
> >> >
> >> >
> >> > 83:00.0 Co-processor: Intel Corporation DH895XCC Series QAT
> >> > Subsystem: Intel Corporation Device 
> >> > Physical Slot: 2
> >> > Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
> >> > Stepping- SERR+ FastB2B- DisINTx+
> >> > Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
> >> > SERR-  >> > Latency: 0, Cache Line Size: 32 bytes
> >> > Interrupt: pin A routed to IRQ 35
> >> > NUMA node: 1
> >> > Region 0: Memory at fb90 (64-bit, prefetchable) [size=512K]
> >> > Region 2: Memory at fbd4 (64-bit, non-prefetchable) [size=256K]
> >> > Region 4: Memory at fbd0 (64-bit, non-prefetchable) [size=256K]
> >> > Capabiliti

Re: Alg errors with Intel QAT Card

[Neil Horman]

On Mon, Jun 12, 2017 at 03:52:07PM +, Benedetto, Salvatore wrote:
> Hi Raj,
> 
> I've compiled and tested kernel 4.12.0-rc4 and I can't reproduce your issue.
> Are you seeing any of this with a previous kernel version? If not, git bisect 
> might
> help us finding the root-cause.
> Have you tried with another platform/hw?
> 
> Regards,
> Salvatore
> 
Try a soft reboot and see if the error clears up.  This looks a bit reminscient
of some firmware errors we've been chasing down
Neil

> > -Original Message-
> > From: linux-crypto-ow...@vger.kernel.org [mailto:linux-crypto-
> > ow...@vger.kernel.org] On Behalf Of Raj Ammanur
> > Sent: Friday, June 9, 2017 7:37 PM
> > To: Linux Crypto Mailing List 
> > Subject: Alg errors with Intel QAT Card
> > 
> > Hi
> > 
> > I am seeing the below errors after  installing an Intel QAT card
> > and loading the upstreamed qat_dh895xcc and intel_qat modules.
> > 
> > Have others seen similar errors and know if this is a known issue
> > and a fix exists or know whats going on ? This is with 4.12.0-rc4+
> > version of the kernel.
> > 
> > Any help is sincerely appreciated.
> > 
> > thanks
> > --Raj
> > 
> > 
> > [3.639046] dh895xcc :00:0b.0: qat_dev0 started 12 acceleration 
> > engines
> > [4.168887] alg: skcipher-ddst: Test 5 failed (invalid result) on
> > encryption for qat_aes_cbc
> > [4.217866] alg: skcipher-ddst: Chunk test 1 failed on encryption
> > at page 0 for qat_aes_ctr
> > [4.282042] alg: skcipher: Test 4 failed (invalid result) on
> > encryption for qat_aes_xts
> > [4.395210] alg: akcipher: test 1 failed for qat-rsa, err=-22
> > [root@dhcp-swlab-681 ~]# dmesg | grep -i alg
> > [1.499336] alg: No test for pkcs1pad(rsa,sha256)
> > (pkcs1pad(rsa-generic,sha256))
> > [2.562511] SELinux:  Class alg_socket not defined in policy.
> > [4.168887] alg: skcipher-ddst: Test 5 failed (invalid result) on
> > encryption for qat_aes_cbc
> > [4.217866] alg: skcipher-ddst: Chunk test 1 failed on encryption
> > at page 0 for qat_aes_ctr
> > [4.282042] alg: skcipher: Test 4 failed (invalid result) on
> > encryption for qat_aes_xts
> > [4.367682] alg: akcipher: encrypt test failed. Invalid output
> > [4.395210] alg: akcipher: test 1 failed for qat-rsa, err=-22
> > [4.431827] alg: dh: generate public key test failed. Invalid output
> > [4.431829] alg: dh: test failed on vector 1, err=-22
> > 
> > 
> > 83:00.0 Co-processor: Intel Corporation DH895XCC Series QAT
> > Subsystem: Intel Corporation Device 
> > Physical Slot: 2
> > Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
> > Stepping- SERR+ FastB2B- DisINTx+
> > Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
> > SERR-  > Latency: 0, Cache Line Size: 32 bytes
> > Interrupt: pin A routed to IRQ 35
> > NUMA node: 1
> > Region 0: Memory at fb90 (64-bit, prefetchable) [size=512K]
> > Region 2: Memory at fbd4 (64-bit, non-prefetchable) [size=256K]
> > Region 4: Memory at fbd0 (64-bit, non-prefetchable) [size=256K]
> > Capabilities: [b0] MSI: Enable- Count=1/1 Maskable+ 64bit+
> > Address:   Data: 
> > Masking:   Pending: 
> > Capabilities: [60] MSI-X: Enable+ Count=33 Masked-
> > Vector table: BAR=2 offset=0003b000
> > PBA: BAR=2 offset=0003b800
> > Capabilities: [6c] Power Management version 3
> > Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-
> > ,D3cold-)
> > Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
> > Capabilities: [74] Express (v2) Endpoint, MSI 00
> > DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s <128ns, L1 <1us
> > ExtTag+ AttnBtn- AttnInd- PwrInd- RBE+ FLReset+ SlotPowerLimit 0.000W
> > DevCtl: Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
> > RlxdOrd- ExtTag+ PhantFunc- AuxPwr- NoSnoop+ FLReset-
> > MaxPayload 256 bytes, MaxReadReq 1024 bytes
> > DevSta: CorrErr+ UncorrErr- FatalErr- UnsuppReq+ AuxPwr- TransPend-
> > LnkCap: Port #0, Speed 5GT/s, Width x16, ASPM L0s, Exit Latency L0s
> > <512ns, L1 unlimited
> > ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp-
> > LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk-
> > ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
> > LnkSta: Speed 5GT/s, Width x16, TrErr- Train- SlotClk- DLActive-
> > BWMgmt- ABWMgmt-
> > DevCap2: Completion Timeout: Range B, TimeoutDis+, LTR-, OBFF Not
> > Supported
> > AtomicOpsCap: 32bit- 64bit- 128bitCAS-
> > DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR-, OBFF
> > Disabled
> > AtomicOpsCtl: ReqEn-
> > LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis-
> >  Transmit Margin: Normal Operating Range, EnterModifiedCompliance-
> > ComplianceSOS-
> >  Compliance De-emphasis: -6dB
> > LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-,
> > EqualizationPhase1-
> >  EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
> > Capabilities: [100 v1] Advanced Error Reporting
> > 

[PATCH] QAT: Fix uninitialized variable in qat driver

[Neil Horman]

Hit a warning when building QAT, indicating that sz_out might be uninitalized
before use.  Looks like if you hit an error path and jump to err: you might find
yourself trying to unmap an arbirarily long dma region.  Its safe on intel since
intel defines the invalid dma address as zero, but other arches don't, and if
qat makes its way to one of those, that can cause all sorts of corruption.

Fix is pretty easy, just init sz_out to zero, and gate the unmapping on sz_out
being non-zero

Signed-off-by: Neil Horman nhor...@tuxdriver.com
CC: Herbert Xu herb...@gondor.apana.org.au
CC: David S. Miller da...@davemloft.net
CC: Tadeusz Struk tadeusz.st...@intel.com
CC: qat-li...@intel.com (open list:QAT DRIVER)
---
 drivers/crypto/qat/qat_common/qat_algs.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/qat_algs.c 
b/drivers/crypto/qat/qat_common/qat_algs.c
index 067402c..35ab752 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -667,8 +667,9 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance 
*inst,
dma_addr_t blp;
dma_addr_t bloutp = 0;
struct scatterlist *sg;
-   size_t sz_out, sz = sizeof(struct qat_alg_buf_list) +
-   ((1 + n + assoc_n) * sizeof(struct qat_alg_buf));
+   size_t sz_out = 0;
+   size_t sz = sizeof(struct qat_alg_buf_list) +
+ ((1 + n + assoc_n) * sizeof(struct qat_alg_buf));
 
if (unlikely(!n))
return -EINVAL;
@@ -793,7 +794,7 @@ err:
dma_unmap_single(dev, buflout-bufers[i].addr,
 buflout-bufers[i].len,
 DMA_BIDIRECTIONAL);
-   if (!dma_mapping_error(dev, bloutp))
+   if (sz_out  !dma_mapping_error(dev, bloutp))
dma_unmap_single(dev, bloutp, sz_out, DMA_TO_DEVICE);
kfree(buflout);
}
-- 
2.1.0

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

Re: [v2 PATCH 0/11] rng: New style interface

[Neil Horman]

On Tue, Apr 21, 2015 at 10:44:36AM +0800, Herbert Xu wrote:
 This series converts the crypto_rng interface over to the new
 style.  I'm putting it in quotes because this style has been
 around since 2008.  In fact, RNG was the very last interface
 type added before the introduction of the new style.
 
 Eventually all existing interfaces should be converted over but
 obviously it's taking some time to get there :)
 
 v2 uses kzfree instead of free for the temporary seed.
 
 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
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 


For the ansi_cprng bits
Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH] crypto: RNGs must return 0 in success case

[Neil Horman]

Change the RNGs to always return 0 in success case.

This patch ensures that seqiv.c works with RNGs other than krng. seqiv
expects that any return code other than 0 is an error. Without the
patch, rfc4106(gcm(aes)) will not work when using a DRBG or an ANSI
X9.31 RNG.

For the X9.31 bits:
Acked-by: Neil Horman nhor...@tuxdriver.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 0/2] crypto: AF_ALG: add AEAD and RNG support

[Neil Horman]

On Wed, Jan 14, 2015 at 04:31:44PM +0100, Stephan Mueller wrote:
 Am Mittwoch, 14. Januar 2015, 22:00:11 schrieb Herbert Xu:
 
 Hi Herbert,
 
  On Wed, Jan 14, 2015 at 04:46:31AM -0500, Neil Horman wrote:
   On Wed, Jan 14, 2015 at 04:52:29AM +0100, Stephan Mueller wrote:
Hi,

This patch set adds AEAD and RNG support to the AF_ALG interface
exported by the kernel crypto API. By extending AF_ALG with AEAD and RNG
support, all cipher types the kernel crypto API allows access to are
now accessible from userspace.

 crypto/Kconfig  |   9 +
 crypto/Makefile |   1 +
 crypto/algif_aead.c | 680
  3 files changed,
 690 insertions(+)
 create mode 100644 crypto/algif_aead.c
   
   Where are the RNG bits?
  
  They've already been merged.
 
 Please also see my change log for v7:
 ...
 * RNG: patch dropped as it was applied
 ...
 
 To ensure consistency, I did not change the intro part of my description.
Ah, thanks for the clarification
Acked-by: Neil Horman nhor...@tuxdriver.com

 
  
  Cheers,
 
 
 -- 
 Ciao
 Stephan
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next] crypto: algif_rng - fix sparse non static symbol warning

[Neil Horman]

On Wed, Jan 14, 2015 at 09:14:41AM +0800, weiyj...@163.com wrote:
 From: Wei Yongjun yongjun_...@trendmicro.com.cn
 
 Fixes the following sparse warnings:
 
 crypto/algif_rng.c:185:13: warning:
  symbol 'rng_exit' was not declared. Should it be static?
 
 Signed-off-by: Wei Yongjun yongjun_...@trendmicro.com.cn
Acked-by: Neil Horman nhor...@tuxdriver.com

 ---
  crypto/algif_rng.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
 index 91c06f5..67f612c 100644
 --- a/crypto/algif_rng.c
 +++ b/crypto/algif_rng.c
 @@ -182,7 +182,7 @@ static int __init rng_init(void)
   return af_alg_register_type(algif_type_rng);
  }
  
 -void __exit rng_exit(void)
 +static void __exit rng_exit(void)
  {
   int err = af_alg_unregister_type(algif_type_rng);
   BUG_ON(err);
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 0/2] crypto: AF_ALG: add AEAD and RNG support

[Neil Horman]

On Wed, Jan 14, 2015 at 04:52:29AM +0100, Stephan Mueller wrote:
 Hi,
 
 This patch set adds AEAD and RNG support to the AF_ALG interface
 exported by the kernel crypto API. By extending AF_ALG with AEAD and RNG
 support, all cipher types the kernel crypto API allows access to are
 now accessible from userspace.
 
  crypto/Kconfig  |   9 +
  crypto/Makefile |   1 +
  crypto/algif_aead.c | 680 
 
  3 files changed, 690 insertions(+)
  create mode 100644 crypto/algif_aead.c
 
 -- 
 2.1.0
 
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
Where are the RNG bits?
Neil

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

Re: [PATCH v2 00/25] Multiple changes to crypto/ansi_cprng.c

[Neil Horman]

On Mon, Dec 15, 2014 at 11:46:15AM +0100, Stephan Mueller wrote:
 Am Montag, 15. Dezember 2014, 05:21:49 schrieb George Spelvin:
 
 Hi George,
 
   Ah, now I see it. Yes, all AES 128 are covered.
   
   What about AES 192 and 256?
  
  The implementation doesn't support them, and I didn't add them.
 
 Sorry, my bad.  :-)
 
 Then, I think the updated implementation matches with the spec.
With that then, I'm really fine with the changes given that they pass the NIST
tests.

Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH v2 25/25] crypto: ansi_cprng - If non-deterministic, don't buffer old output

[Neil Horman]

On Mon, Dec 08, 2014 at 11:43:13AM -0500, George Spelvin wrote:
  Wait, I'm confused. You mention in this note that this is an RFC patch, but 
  not
  anywhere else in the series.  Are you proposing this for inclusion or not?
 
 Er, in the 0/25, I mentioned that I put the least certain stuff last,
 and in particular I wasn't sure if the the last three patches were wanted
 or not:
 
  Pending issues:
  * Is non-deterministic mode (last three patches) wanted?
 
 I certainly wouldn't be unhappy if they went in, but with the comment
 clarification just before, I wouldn't be unhappy if they didn't, either.
 
 They're If we wanted to do this, this is how it could be done.  Is this
 something we want to do?
 
 Sorry if my motivations are confusing.  I did indeed start with wanting
Not your motivations, just the posting mechanics.  If you just want to discuss a
patch, and aren't yet proposing it for inclusion, you should put RFC in the
prefix of every patch header.

 to add the seeding because I misunderstood the comments: I thought
 this was claiming to be X9.31 *and* I haven't seen the later versions
 of the standaed (which you have) that back off on the requirements for
 the DT[] vector.
 
 Since you've patiently explained both of those to me, I'm more interested
 in the other, more generic code cleanups.
 
 You also sent me two detailed explanations of the consequences of making
 the generator non-determinsitic in a way that gave me a general impression
 of disliking of the idea.  So I've been weaning myself off the idea.
 
Not particularly opposed to the idea, I just know that several use cases rely on
deterministic behavior for those entities that share the secret information, so
I need to be sure that the deterministic behavior remains and is the default.

 I put those patches at the end so they can easily be dropped from the series.
 
 Or, as I also mentioned, simply postponed until there's been more discussion. 
  
 Since that's an actual semantic change, collecting a few other opinions
 would be valuable.
I'll look at this series in detail shortly.
Neil

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

Re: [PATCH 00/17] Multiple changes to crypto/ansi_cprng.c

[Neil Horman]

On Wed, Dec 03, 2014 at 03:27:53PM -0500, George Spelvin wrote:
  So, generally speaking I'm ok with most of this I think,
 
 One open issue... I thought you had said that the non-determinsitic
 version was not in the current X9.31 and I had the impression that it
 wasn't wanted.  I've got reorganized patch series that gets rid of that
 and just tweaks the comments.
 
I presume you're referring to the deterministic DT vector here?  It is currently
in the implementation, and I personally have no need for a non-deterministic
variant.  I'm fine if you add it as long as the default remains the same as it
currently is.

 I'm happy to put it back, of course.  Or just hold off until Herbert
 chimes in with an opinion.
 
 As an example of the reorganization, now the comment for patch 4 in the
 series is a bit clearer:
 
 crypto: ansi_cprng - Eliminate ctx-I and ctx-last_rand_data
 
 Careful use of the other available buffers avoids the need for
 these, shrinking the context by 32 bytes.
 
 Neither the debug output nor the FIPS-required anti-repetition check
 are changed in the slightest.
 
 (That's because I change the debug output in patches 2 and 3, to make
 this more-sensitive patch easier to review.)
 
  but before it goes anywhere, it really needs to pass the NIST and FIPS
  test vectors.  Can you do that please and post the results?
 
 It of course passes the ones in testmgr.h, and I can add the others from
 http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf
 and
 http://csrc.nist.gov/groups/STM/cavp/documents/rng/rngtestvectors.zip
 
 I didn't know there were separate FIPS test vectors for this PRNG;
 can you give me a clue where to find them?
 
 
 I'm also not sure what the results look like.  As I mentioned previously,
 I have been unable to figure out how to make the tcrypt code print anything
 in the event of a successful test, so its output is the empty string.
 
 I am using my own version which prints cprng: Test %d passed, and
 I can turn debugging on, but the 1-round MCT produces an annoying
 amount of output in that case.
 
 (Note to self: teach test_cprng to test odd-sized reads, since that was
 a previous bug source and I'm touching that code some more.)
 
  I'm fine with changing the output, as I don't think anything
  particularly relies on the format, but I can't speak for others.
 
 The current debug output for the first 5 testmgr.h tests (the 6th is
 omitted) is follows, but obviously things get reconfirmed right at
 the end.
 
 getting 16 random bytes for context 88042ce41b18
 Calling _get_more_prng_bytes for context 88042ce41b18
 DT = e6b3be782a23fa62d71d4afbb0e922f9
 V = 8000
 I = 92640cf08d302f2550ea3d73d1985385
 V^I = 12640cf08d302f2550ea3d73d1985385
 R = 59531ed13bb0c05584796685c12f7641
 R^I = cb371221b680ef70d4935bf610b725c4
 V' = 2ac489ad47640b6d86380229e769adc3
 DT' = e6b3be782a23fa62d71d4afbb0e922fa
 Returning new block for context 88042ce41b18
 returning 16 from get_prng_bytes in context 88042ce41b18
 cprng: Test 0 passed
 getting 16 random bytes for context 88042ce41b18
 Calling _get_more_prng_bytes for context 88042ce41b18
 DT = e6b3be782a23fa62d71d4afbb0e922fa
 V = c000
 I = b30a8cba1c4cd3a14af7d9db9488f5f0
 V^I = 730a8cba1c4cd3a14af7d9db9488f5f0
 R = 7c222cf4ca8fa24c1c9cb641a9f3220d
 R^I = cf28a04ed6c371ed566b6f9a3d7bd7fd
 V' = fcbd61e7c51dd167624c56cb97b4c66d
 DT' = e6b3be782a23fa62d71d4afbb0e922fb
 Returning new block for context 88042ce41b18
 returning 16 from get_prng_bytes in context 88042ce41b18
 cprng: Test 1 passed
 getting 16 random bytes for context 88042ce41b18
 Calling _get_more_prng_bytes for context 88042ce41b18
 DT = e6b3be782a23fa62d71d4afbb0e922fb
 V = e000
 I = d761699cc3de4a90234c62eec2479cd9
 V^I = 3761699cc3de4a90234c62eec2479cd9
 R = 8aaa003966675be529142881a94d4ec7
 R^I = 5dcb69a5a5b911750a584a6f6b0ad21e
 V' = ef2c1fbf609a68f8fe5110636bf4bf6a
 DT' = e6b3be782a23fa62d71d4afbb0e922fc
 Returning new block for context 88042ce41b18
 returning 16 from get_prng_bytes in context 88042ce41b18
 cprng: Test 2 passed
 getting 16 random bytes for context 88042ce41b18
 Calling _get_more_prng_bytes for context 88042ce41b18
 DT = e6b3be782a23fa62d71d4afbb0e922fc
 V = f000
 I = 048ecb25943e8c31d614cd8a23f13f84
 V^I = f48ecb25943e8c31d614cd8a23f13f84
 R = 88dda456302423e5f69da57e7b95c73a
 R^I = 8c536f73a41aafd4208968f45864f8be
 V' = 48893b71bce400b65e21ba378a0ad570
 DT' = e6b3be782a23fa62d71d4afbb0e922fd
 Returning new block for context 88042ce41b18
 returning 16 from get_prng_bytes in context 88042ce41b18
 cprng: Test 3 passed
 getting 16 random bytes for context 88042ce41b18
 Calling _get_more_prng_bytes for context 88042ce41b18
 DT = e6b3be782a23fa62d71d4afbb0e922fd
 V = f800
 I = 3a6a1754bdf6f844e53662990cadc492
 V^I 

Re: [PATCH 03/17] crypto: ansi_cprng - Eliminate ctx-I

[Neil Horman]

On Tue, Dec 02, 2014 at 03:03:38PM -0500, George Spelvin wrote:
  I'm only ok with removing I if you can continue to be able to output it.
  given that I is listed as part of the test sequences that NIST provides,
  I'd like to be able to compare the values.
 
 I can do that easily, but I can't print the *input* I, which
 is the result of encrypting the previous DT, as it's thrown
 away earlier.
 
 You'd have to look further back in the debug messages to find it.
 
 Is changing the format of the debug messages okay?  I'd like the debug
 messages to describe the code, but I don't know if you have something
 that parses the current output.
 
I'm fine with changing the output, as I don't think anything particularly relies
on the format, but I cant' speak for others
Neil

 
 The test output I see on p. 33 of
 http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf
 doesn't include I.  Can you point me to a sample that includes I?
 
 It might be best to more significantly  rework the debug messages to
 resemble the NIST test vectors.
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/17] crypto: ansi_cprng - Shrink rand_read_pos flags

[Neil Horman]

On Tue, Dec 02, 2014 at 03:28:17PM -0500, George Spelvin wrote:
  --- a/crypto/ansi_cprng.c
  +++ b/crypto/ansi_cprng.c
  @@ -51,9 +51,9 @@ struct prng_context {
 unsigned char rand_data[DEFAULT_BLK_SZ];
 unsigned char DT[DEFAULT_BLK_SZ];
 unsigned char V[DEFAULT_BLK_SZ];
  -  u32 rand_read_pos;  /* Offset into rand_data[] */
  +  unsigned char rand_read_pos;/* Offset into rand_data[] */
 
  u8 please.  Also, not sure if this helps much, as I think the padding
  will just get you back to word alignment on each of these.
 
 I noticed the unsigned char vs u8 issue, but didn't make the change
 as I didn't think the readailility improvement was worth the code churn.
 
You just sent a 17 patch series of clean up and were worried about code churn
converting an unsigned char to a u8?


 But I'd be happy to clean that up, too!
 
 Should I convert all the buffers and function prototypes, or is there
 some criterion for distinguishing which gets which?   (E.g. buffers are
 unsigned char but control variables are u8.)
 
If you want to sure.  u8 probably makes more sense for the buffers here as our
intent is to treat them as an array of byte values.

 And actually, you do win.  spinlock_t is 16 bits on x86,
 and the buffers are all 16 bytes.   (80 bytes before my earlier
 patches, 48 bytes after.)
 
 So the the structure goes from:
 
 32-bit64-bit  Variable
 OffsetSizeOffset  Size
  020  2   spinlock_t prng_lock
  216   2  16  unsigned char rand_data[16]
 1816  18  16  unsigned char DT[16]
 3416  34  16  unsigned char V[16]
 502   50  2   (alignemnt)
 524   52  4   u32 rand_read_pos
 564   56  8   struct crypto_cipher *tfm
 604   64  4   u32 flags
   68  4   (alignment)
 6472  (structure size)
 
 to
 
 32-bit64-bit  Variable
 OffsetSizeOffset  Size
 3416  34  16  unsigned char V[16]
 501   50  1   u8 rand_read_pos
 511   51  1   u8 flags
   52  4   (alignment)
 524   56  8   struct crypto_cipher *tfm
 5664  (structure size)
 
 You still get 4 bytes of alignment padding on x86-64, but given that
 the structure has 60 bytes of payload, that's the minimum possible.
 
 You save 6 bytes of variables and 2 bytes of padding on both
 32- and 64-bit systems, for a total of 8 bytes, and that's enough
 to knock you into a smaller slab object bin on 64-bit.
 
 
 I forget where I read the terminology, but the most efficient
 wat to pack a structure is in an organ pipe configuraiton where
 the biggest (in terms of *alignment*) members are on the outside
 and the structre and the smaller elements are on the inside.
 Putting a 32-bit flags after a 64-bit pointer violates that.
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/17] Multiple changes to crypto/ansi_cprng.c

[Neil Horman]

On Tue, Dec 02, 2014 at 03:33:14AM -0500, George Spelvin wrote:
 Thank you all for your hospitality to my amateurish efforts.
 
 Given that this all started with me reading the code in an attempt to
 understand it, it is likely that some of the problems I address are
 actually misunderstandings on my part.  If all I get from this patch series
 is some enlightenment, that's okay.
 
 It's even more likely that some parts contain the germ of a good idea,
 but will need considerable rework to be acceptable.  I look forward
 to that too.
 
 Expecting that much more work will be needed, I've run the testmgr.h
 test vectors on this code, but haven't desk-checked it as thoroughly as
 security-sensitive code should be before final acceptance.  If the ideas
 pass muster, I'll double-check the implementatoin details.
 
 Really, I'm just trying to understand the code.  Patches are just
 a very specific way of talking about it.
 
 Here's an overview of the series.  It's a lot of code cleanup, and
 a bit of semantic change.
 
 [01/17] crypto: ansi_cprng - Rename rand_data_valid more sensibly
   I find it confusing, so I rename it to rand_data_pos
 [02/17] crypto: ansi_cprng - Eliminate ctx-last_rand_data
   Shrink the context structure.
 [03/17] crypto: ansi_cprng - Eliminate ctx-I
   Shrink it some more.
 [04/17] crypto: ansi_cprng - simplify xor_vectors() to xor_block()
   We don't need a size parameter.
 [05/17] crypto: ansi_cprng - Add const annotations to hexdump()
   I like const.
 [06/17] crypto: ansi_cprng - Eliminate unused PRNG_FIXED_SIZE flag
   It's dead code ACAICT.
 [07/17] crypto: ansi_cprng - Shrink rand_read_pos  flags
   A little more context shrinking.
 [08/17] crypto: ansi_cprng - Require non-null key  V in reset_prng_context
   Sue to the PRNG_NEED_RESET flag, cprng_init() doesn't need to call
   reset_prng_context) directly.
 [09/17] crypto: ansi_cprng - Clean up some variable types
   Try to be more consistent about signed vs. unsigned.
 [10/17] crypto: ansi_cprng - simplify get_prng_bytes
   Code shrink  simplification.
 [11/17] crypto: ansi_cprng - unroll _get_more_prng_bytes
   Slight object code size savings, and definite readability improvement.
 [12/17] crypto: ansi_cprng - Create a block buffer data type
   union { u8 bytes[16]; u32 ints[4]; u64 longs[2]; }, sort of.
 [13/17] crypto: ansi_cprng - If DT is not provided, use a fresh timestamp
   This is the big semantic change.  I'm rather proud of the use
   of get_random_int() as a timestamp, but feedback is definitely
   solicited!
 [14/17] crypto: ansi_cprng - If DT is omitted, don't buffer old output
   Not sure if this is desirable or not.
 [15/17] crypto: testmgr - Teach test_cprng to handle non-default seed sizes
   I consider this a latent bug that patch 17 sould expose.
 [16/17] crypto: testmgr - Merge seed arrays in struct cprng_testvec
   I think this is a better way of solving the preceding problem,
   but it's more intrusive.  All the consts I add are not a critical
   part of the patch, but an example of what I'd like to do to the
   rest of the code.
 [17/17] crypto: ansi_cprng - Shrink default seed size
   This makes (some amount of) true entropy the default.
 
So, generally speaking I'm ok with most of this I think, but before it goes
anywhere, it really needs to pass the NIST and FIPS test vectors.  Can you do
that please and post the results?
Neil

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

Re: Is ansi_cprng.c supposed to be an implmentation of X9.31?

[Neil Horman]

On Mon, Dec 01, 2014 at 11:55:18PM -0500, George Spelvin wrote:
  The other one, which I have to think *very* hard about, is whether
  using the same seed material as /dev/random in this much weaker
  cryptographic construction will introduce a flaw in *it*.
 
  I have no idea what you are referring to here. The caller is requred to 
  seed the DRNG. Typically that is done with a call to get_random_bytes. 
  As Neil mentioned, the X9.31 DRNG implementation does not seed itself.
 
 Er, yes it does.  Not the key, but the IV vector V[].
 
 Here's the closest thing I can find on-line to a direct quote
 from the ANSI Spec:
 
 http://www.untruth.org/~josh/security/ansi931rng.PDF
 
 Let DT be a date/time vector which is updated on each iteration.
 
 This timestamp is a source of continuous reseed material.  The spec
 doesn't impose specific resolution requirements, but it's hopefully
 obvious that the finer, the better.  The goal is a vector which changes
 per iteration.
 
That is an old version.  The updated version (published in 2005), and specified
in the ansi_cprng.c file removes that language.

 There are limitations due to its finite entropy and lack of catastrophic
 reseeding, as Kelsey  Schneier pointed out:
 
 https://www.schneier.com/paper-prngs.html
 
 but it is intended as an entropy source.
 
Again, that language is removed in the more recent version.  Using DT as an
entropy source, and updating it on each iteration also destroys the predictive
nature of the cprng when two peers use it to communicate.  That is to say, if a
DT vector is updated every iteration, and the resultant R value is used as a key
to encrypt data, the validitiy of that key at a peer host using an identically
seeded cprng to decrypt is limited by the granularity of the DT value.  That is
to say, if you update DT every second in cprng, an entity that knows the secret
key can only decrypt that data up to a second after its encryption, unless the
decrypting entity also knows at exactly what time the data was encrypted.  If
you share the DT value, any entropy it provides becomes worthless, as it becomes
widely known.

The long and the short of it is that, if you want a cprng who's output can be
predicted by any entity with the IV and KEY values, then DT has to be known
initially and updated in a predictable fashion that is independent of the data
being transmitted.  Using a real date/time vector can't do that.

 Hopefully very soon I'll have a patch series to post.  (Unfortunately,
 I just managed to oops my kernel and need to reboot to continue testing.)
 
 If I don't do much more, it'll be patch 13/17 in the series.  I think
 the use of get_random_int() is a good compromise between the raw
 random_get_entropy() timestamp and the (too heavy) get_random_bytes().
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Is ansi_cprng.c supposed to be an implmentation of X9.31?

[Neil Horman]

On Tue, Dec 02, 2014 at 12:39:30AM -0500, George Spelvin wrote:
  See Documentation/DocBook/crypto-API.tmpl in the cryptodev-2.6 tree. 
  There you will find tons of documentation (which will be merged during 
  3.19-rc1)
 
 Yes, I've been reading that.  It certainly helps a great deal, but
 still leaves me with some significant questions.
 
 I started researching the crypto layer when I proposed using Dan
 Bernstein's SipHash elsewhere in the kernel and someone asked for a
 crypto API wrapper for it.  That seemed a simple enough request to me,
 but it's been a deeper rabbit hole than I expected.
 
 I started reading the code to another keyed hash, michael_mic, as a model,
 but I'm stil trying to understand the intended difference between struct
 crypto_shash and struct shash_desc, and in particular why both have
 a copy of the key.  The SHASH API documentation at
 
 https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/tree/include/crypto/hash.h
 
 isn't particularly enlightening.  If the crypto_shash were entirely
 read-only and the shash_desc were the entire volatile state, that would
 make sense, but as it is I'm confused about the design intent.
 
Not sure what you're concerned about, or what you're even referencing.  A
struct crypto_shash is the object retured by crypto_alloc_shash, and represents
an instance of a synchronous hash algorithm:
struct crypto_shash {
unsigned int descsize;
struct crypto_tfm base;
};

The key is stored in the base.__crt_ctx part of the structure in a algorithm
specific manner.

The shash_desc structure represents a discrete block of data that is being
hashed.  It can be updated with new data, reset, finalized, etc.  It only points
to the crypto_hash structure that it is associated with (as it must, given that
the crypto layer needs to know what algorithm to use to hash the data and what
key to use).  But it doesn't store a second copy of the key on its own.

 (On a related point, the general lack of const declarations throughout the
 crypto layer has been a source of frustration.)
 
So fix it.  Making large claims about what frustrates you about the code without
producing any fixes doesn't make people listen to you.
 
 The other big issue I'm struggling with is how to get the tcrypt.ko module
 to print ansi_cprng has passed its tests.  All it has produced for me
 so far is a few kilobytes of dmesg spam about ciphers which aren't even
 configured in my kernel.
 
The tests only print out a pass fail notification in fips mode, that seems
pretty clear if you look at the alg_test function.

 After a few hours of trying to figure out what the alg and type parameters
 do, I gave up and cut and pasted the tests into prng_mod_init().

They're used to differentiate algorithms that can be used for multiple purposes.
See the CRYPTO_ALG_TYPE_* defines in crypto.h.  For the CPRNG, they do nothing
since an RNG is only an RNG.

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

Re: [PATCH 02/17] crypto: ansi_cprng - Eliminate ctx-last_rand_data

[Neil Horman]

On Tue, Dec 02, 2014 at 03:35:50AM -0500, George Spelvin wrote:
 It's simply not necessary.
 
 Signed-off-by: George Spelvin li...@horizon.com
NACK

The assumption that its not needed is incorrect.  In fips mode its explicitly
needed to validate that the rng isn't reproducing identical random data.

Neil

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

Re: [PATCH 03/17] crypto: ansi_cprng - Eliminate ctx-I

[Neil Horman]

On Tue, Dec 02, 2014 at 03:37:07AM -0500, George Spelvin wrote:
 It's also not necessary.  We do have to change some debugging
 output.
 
 Signed-off-by: George Spelvin li...@horizon.com
 ---
  crypto/ansi_cprng.c | 39 ---
  1 file changed, 20 insertions(+), 19 deletions(-)
 
I'm only ok with removing I if you can continue to be able to output it.  given
that I is listed as part of the test sequences that NIST provides, I'd like to
be able to compare the values.
Neil

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

Re: [PATCH 07/17] crypto: ansi_cprng - Shrink rand_read_pos flags

[Neil Horman]

On Tue, Dec 02, 2014 at 03:43:13AM -0500, George Spelvin wrote:
 rand_read_pos is never more than 16, while there's only 1 flag
 bit allocated, so we can shrink the context a little.
 
 Signed-off-by: George Spelvin li...@horizon.com
 ---
  crypto/ansi_cprng.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 They're also reordered to avoid alignment holes.
 
 diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
 index 93ed00f6..f40f54cd 100644
 --- a/crypto/ansi_cprng.c
 +++ b/crypto/ansi_cprng.c
 @@ -51,9 +51,9 @@ struct prng_context {
   unsigned char rand_data[DEFAULT_BLK_SZ];
   unsigned char DT[DEFAULT_BLK_SZ];
   unsigned char V[DEFAULT_BLK_SZ];
 - u32 rand_read_pos;  /* Offset into rand_data[] */
 + unsigned char rand_read_pos;/* Offset into rand_data[] */
u8 please.  Also, not sure if this helps much, as I think the padding will just
get you back to word alignment on each of these.

 + unsigned char flags;
   struct crypto_cipher *tfm;
 - u32 flags;
  };
  
  static int dbg;
 -- 
 2.1.3
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Is ansi_cprng.c supposed to be an implmentation of X9.31?

[Neil Horman]

On Fri, Nov 28, 2014 at 06:23:51PM -0500, George Spelvin wrote:
 I've been trying to understand the crypto layer, and it's a bit of a
 struggle because I'm trying to learn how it's supposed to work by
 reading the code, and I keep finding code I want to fix.
 
Patches welcome.

 ansi_cprng.c is the current itch I'm eager to scratch.
 
 Other than enough implementation stupidities to make me scream
 (particularly the rand_data_valid variable name which is actually a
Its actually a counter of the number of valid random data bytes in the buffer
being returned to a caller, as well as an index into the internal buffer from
which to draw fresh random data.  Sorry if you don't get that, but it seems
pretty clear.

 count of INvalid data, and  keeping 5 blocks of state, including sensitive
 previous output, when only 3 are needed), one thing I can't help noticing
Not sure where you're getting that from, only 1 block of random data is stored
at any one time to return to a caller

 is that this is definitely NOT conformant with the X9.17/X9.31 spec.
 
This is the document it was based of off:
http://csrc.nist.gov/groups/STM/cavp/documents/rng/931rngext.pdf

From my read, it seems to be in complete compliance.

 That's because the spec requires a timestamp for each output block
 to provide additional entropy, and a counter won't cut it.
 
The document places no constrints on the value or progression of DT.  As such a
counter is as valid as any other implementation.  You're welcome to enhance that
however, as I said, patches welcome.

 I'm fixing the obvious things, but on this point, I have two choices:
 
 1. Add some comments clarifying that the Based on part of the header
is anything but a claim of compliance; those specs are for an RNG,
while this is a PRNG. 
Please read more closely, the header clearly states this is a PRNG
implementation, and a quick google search of the terms in the header bring up
the document referenced above, with which this cprng is in compliance with.

  And probably delete all the FIPS stuff, as
there's no spec to claim compliance with.  Or
Maybe do some research before making big claims like this:
http://csrc.nist.gov/publications/fips/fips140-2/fips1402annexc.pdf

Its just a draft, but digging through the NIST site will bring up the approved
version.  Both show that a 3 DES CPRNG based on ANSI X9.31 is valid, and
provides  a reference to the paper above as the implementation guideline.

 2. Fix the code to use random_get_entropy() and jiffies for the
DT seed vector.
 
Sure, knock yourself out.  I don't consider it more or less valid to do so, but
patches are welcome.

 In the latter case, I'd have to leave the current deterministic code as
 an option for self-testing, but I'd drop the recommended seedsize
 to DEFAULT_PRNG_KSZ + DEFAULT_BLK_SZ (one key and one IV), and have
 an internal flag indicating whether to use an incrementing DT vector
 or generate it fresh.
Yup.  Strictly speaking the API is in-kernel only, so you don't really have to
worry about handling backwards compatibility, but if you don't allow for DT to
be specified as an initial value, you won't be able to validate against any of
the test vectors that NIST provides:
http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf

I will also point out that CPRNG's are designed such that peers communicating
with a cprng are expected to be able to predect the cprng output, which implies
that the DT value needs to remain predictable as well.  Using an actual date
time vector is going to make communication like that very unstable if there is
even a little clock drift on either system.  As such, while its less entropy,
using a simple arbitrary vector with an increment on each random data generation
leads to greater stability and predictability for those with the key.  The data
provided in the validation test in Appendix B.1.5 of the above document supports
that, as the DT value is arbitrary and incremented by one on each iteration.

 
 If some code (like the current self-test code) provides an extra
 DEFAULT_BLK_SZ of seed material, it would go into determinsitic mode,
 but if it's missing, DT would be generated dynamically.
 
Sure, patches welcome.

 But that's a question of design intent, and I can't intuit that from the
 code.  Can someome enlighten me as to which option is preferred?
 
Definately keep the ability to support external setting of DT, as you can't pass
any validation tests without it.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Is ansi_cprng.c supposed to implement X9.17/X9.31's RNG?

[Neil Horman]

On Sat, Nov 29, 2014 at 12:26:49PM -0500, George Spelvin wrote:
 Sorry for the duplicate; I had a crash and I thought the mail was lost.
 First message was not quite finished, second is a rewrite from scratch.
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
Responded to your first note
Neil

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

Re: Is ansi_cprng.c supposed to be an implmentation of X9.31?

[Neil Horman]

On Sat, Nov 29, 2014 at 02:32:05PM -0500, George Spelvin wrote:
  Other than enough implementation stupidities to make me scream
  (particularly the rand_data_valid variable name which is actually a
 
  Its actually a counter of the number of valid random data bytes in the 
  buffer
  being returned to a caller, as well as an index into the internal buffer 
  from
  which to draw fresh random data.  Sorry if you don't get that, but it seems
  pretty clear.
 
 As you can see, I ended up choosing less abrasive wording in the version I
 *thought* was public; this got launched into the void while in draft form.
 Sorry about that.
 
 Oh, its use as an index into the read_data array is clear enough;
 it's just that the fact that the number of valid bytes in that
 array is DEFAULT_BLOCK_SZ - ctx-rand_data_valid seems obviously
 backward to me.
 
 You'd think ctx-rand_data_valid = 0 would mean no data is valid;
 force update cycle next access, but nope...
 
  is that this is definitely NOT conformant with the X9.17/X9.31 spec.
  
  This is the document it was based of off:
  http://csrc.nist.gov/groups/STM/cavp/documents/rng/931rngext.pdf
 
 Ah, I actually read that, but I didn't remember that the Based on
 wording is a direct quote.
 
 If you go to the original ANSI specs (which I've read in the past,
 but din't have a web-accessible copy to link to), they're a bit
 more explicit on the point.
 
  Please read more closely, the header clearly states this is a PRNG
  implementation, and a quick google search of the terms in the header bring 
  up
  the document referenced above, with which this cprng is in compliance with.
 
 Yes, it was quite clear that a strict reading of the comments said that
 it was a PRNG, but I lost track of the fact that Random Number Generator
 Based on ANSI X9.31 Appendix A.2.4 was NIST's wording that was being
 quoted, and was left with the impression that compliance to the ANSI spec
 (which *does* call for a high resolution timestamp) was being implied.
 
 I either wanted to provide the implied compliance or clarify the
 absence of it.
 
  Sure, knock yourself out.  I don't consider it more or less valid to do so,
  but patches are welcome.
 
 Coming right up!
 
  Definately keep the ability to support external setting of DT, as you
  can't pass any validation tests without it.
 
 Yes, I've already figured that out when studying the impact of such a
 change.  Since there's already special-case handling of with/without
 a DT vector, that seemed the obvious thing to hook into.
 
 The two changes that affected callers, which I didn't feel very confident
 about my understanding of, were:
 1. Changing the recommended seed size, and
Well, you only need to worry about the users of the API in the kernel, for which
I think there is only one currently, so I would say change the seed size if you
want, and make sure the one caller matches it appropriately.  Not saying I agree
with the change (or disagree with it), but I think you don't really need to
worry about it.

 2. Using actual random seed material.
 
Thats really a call for the allocator of a cprng to make.  When you allocate a
cprng instance, you have to reset it before you use it, so you have to provide a
new IV, DT and KEY value anyway.  If the caller wants to provide real random
material, they are prefectly able to.  If you want you can default to using
random data when no seed is provided, but I wouldn't recommend it, since the
random pool can block, and we might be resetting the cprng in a non-blocking
context.  I would say just leave this to the caller.

 The other one, which I have to think *very* hard about, is whether
 using the same seed material as /dev/random in this much weaker
 cryptographic construction will introduce a flaw in *it*.
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] crypto/fips: only panic on bad/missing crypto mod signatures

[Neil Horman]

On Wed, Jul 02, 2014 at 03:37:30PM -0400, Jarod Wilson wrote:
 Per further discussion with NIST, the requirements for FIPS state that
 we only need to panic the system on failed kernel module signature checks
 for crypto subsystem modules. This moves the fips-mode-only module
 signature check out of the generic module loading code, into the crypto
 subsystem, at points where we can catch both algorithm module loads and
 mode module loads. At the same time, make CONFIG_CRYPTO_FIPS dependent on
 CONFIG_MODULE_SIG, as this is entirely necessary for FIPS mode.
 
 v2: remove extraneous blank line, perform checks in static inline
 function, drop no longer necessary fips.h include.
 
 CC: Herbert Xu herb...@gondor.apana.org.au
 CC: David S. Miller da...@davemloft.net
 CC: Rusty Russell ru...@rustcorp.com.au
 CC: Stephan Mueller stephan.muel...@atsec.com
 CC: linux-crypto@vger.kernel.org
 Signed-off-by: Jarod Wilson ja...@redhat.com
Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH] drivers/crypto: Use RCU_INIT_POINTER(x, NULL) in nx/nx-842.c

[Neil Horman]

On Mon, Mar 24, 2014 at 01:01:04AM +0530, Monam Agarwal wrote:
 This patch replaces rcu_assign_pointer(x, NULL) with RCU_INIT_POINTER(x, NULL)
 
 The rcu_assign_pointer() ensures that the initialization of a structure   
 is carried out before storing a pointer to that structure. 
 And in the case of the NULL pointer, there is no structure to initialize. 
 So, rcu_assign_pointer(p, NULL) can be safely converted to 
 RCU_INIT_POINTER(p, NULL)
 
 Signed-off-by: Monam Agarwal monamagarwal...@gmail.com
 ---
  drivers/crypto/nx/nx-842.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/crypto/nx/nx-842.c b/drivers/crypto/nx/nx-842.c
 index 1e5481d..c4fcbf4 100644
 --- a/drivers/crypto/nx/nx-842.c
 +++ b/drivers/crypto/nx/nx-842.c
 @@ -1234,7 +1234,7 @@ static int __exit nx842_remove(struct vio_dev *viodev)
   old_devdata = rcu_dereference_check(devdata,
   lockdep_is_held(devdata_mutex));
   of_reconfig_notifier_unregister(nx842_of_nb);
 - rcu_assign_pointer(devdata, NULL);
 + RCU_INIT_POINTER(devdata, NULL);
   spin_unlock_irqrestore(devdata_mutex, flags);
   synchronize_rcu();
   dev_set_drvdata(viodev-dev, NULL);
 @@ -1285,7 +1285,7 @@ static void __exit nx842_exit(void)
   spin_lock_irqsave(devdata_mutex, flags);
   old_devdata = rcu_dereference_check(devdata,
   lockdep_is_held(devdata_mutex));
 - rcu_assign_pointer(devdata, NULL);
 + RCU_INIT_POINTER(devdata, NULL);
   spin_unlock_irqrestore(devdata_mutex, flags);
   synchronize_rcu();
   if (old_devdata)

This really does't seem right.  rcu_assign_pointer users ACCESS_ONCE to protect
against multiple loads that allow for concurrent read/write use with parallel
rcu_dereference calls, whereas RCU_INIT_POINTER does not.  It also just doesn't
look right.  We've already initalized the pointerin nx842_init, we don't need to
re-initalize it here, we need to assign it to NULL.

Neil

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

Re: [PATCH] crypto: Add soft module dependency to load HW accelerated crypto modules

[Neil Horman]

On Sun, Feb 16, 2014 at 07:41:23AM +0800, Herbert Xu wrote:
 On Fri, Feb 14, 2014 at 06:01:06PM -0800, Tim Chen wrote:
  
  Module A selects CRYPTO_ALG_B in config, which causes the generic
  ALG_B to get built and a module dependency to the generic
  ALG_B to be established.  Then when module A loads,
 
 No, if you select ALG_B then yes it'll get built but it certainly
 won't establish a module dependency on ALG_B.  Only a softdep could
 create such a dependency.
 
My understanding was that the soft dependencies were there to work exactly at
the softdep directive was in modprobe.conf.  That it to say, Module B would
like to have Module A installed, but it won't be fatal if it can't be done.
The more correct example I've seen thus far has been the crct10dif crc
algorithm for x86.  In lib/crc-t10dif we have a crc-t10dif function which makes
use of the generic crc_t10dif_generic function defined in crct10dif_common.c.
However, x86 has an accelerated version of the function defined in the
crct10dif-pcmul module (thats aliased to crct10dif).  So the library funcion in
the kernel lists crct10dif as a soft dependency and modprobe tries to load it,
but just carries on if it fails.

Although as I write that, several things confuse me.  When are module
dependencies meant to be satisfied on behalf of the monolithic kernel, where
that dependecy is defined?  I'm guessing right before the init routine is
called for that code, but I'm not sure.  Also, I'm a bit confused by Marco and
Lukas' statement about how they see soft dependencies working.  I'm not sure
where the requirement to recognize soft dependencies comes from in depmod.

Regards
Neil

 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
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: Add soft module dependency to load HW accelerated crypto modules

[Neil Horman]

On Fri, Feb 14, 2014 at 11:14:37AM -0800, Tim Chen wrote:
 We added the soft module dependency of various crypto algorithm's module alias
  to generic crypto algorithm's module. This loads hardware accelerated
  modules and uses them when available.
 
This is great, but have any of the module load utilties been modified to
recognize and handle soft dependencies in the modinfo section? Last I checked
they hadn't.  Do you plan to add that functionality?

Neil

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

[PATCH] ansi_cprng: Fix off by one error in non-block size request

[Neil Horman]

Stephan Mueller reported to me recently a error in random number generation in
the ansi cprng. If several small requests are made that are less than the
instances block size, the remainder for loop code doesn't increment
rand_data_valid in the last iteration, meaning that the last bytes in the
rand_data buffer gets reused on the subsequent smaller-than-a-block request for
random data.

The fix is pretty easy, just re-code the for loop to make sure that
rand_data_valid gets incremented appropriately

Signed-off-by: Neil Horman nhor...@tuxdriver.com
Reported-by: Stephan Mueller stephan.muel...@atsec.com
CC: Stephan Mueller stephan.muel...@atsec.com
CC: Petr Matousek pmato...@redhat.com
CC: Herbert Xu herb...@gondor.apana.org.au
CC: David S. Miller da...@davemloft.net
---
 crypto/ansi_cprng.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index c0bb377..666f196 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -230,11 +230,11 @@ remainder:
 */
if (byte_count  DEFAULT_BLK_SZ) {
 empty_rbuf:
-   for (; ctx-rand_data_valid  DEFAULT_BLK_SZ;
-   ctx-rand_data_valid++) {
+   while (ctx-rand_data_valid  DEFAULT_BLK_SZ) {
*ptr = ctx-rand_data[ctx-rand_data_valid];
ptr++;
byte_count--;
+   ctx-rand_data_valid++;
if (byte_count == 0)
goto done;
}
-- 
1.8.3.1

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

Re: race condition in crypto larval handling

[Neil Horman]

On Fri, Sep 06, 2013 at 04:20:50PM -0700, Kees Cook wrote:
 Hi,
 
 I've tracked down a race condition and ref counting problem in the
 crypto API internals. We've been seeing it under Chrome OS, but it
 seems it's not isolated to just us:
 
 https://code.google.com/p/chromium/issues/detail?id=244581
 http://marc.info/?l=linux-crypto-vgerm=135429403609373w=2
 https://bugzilla.redhat.com/show_bug.cgi?id=983682
 http://www.mail-archive.com/linux-cifs@vger.kernel.org/msg07933.html
 
 I think I've found the basic origin of the problem.
 crypto_larval_add() has synchronization to make sure only a single
 larval can ever be created. That logic seems totally fine. However,
 this means that crypto_larval_lookup() from two threads can return the
 same larval, which means that crypto_alg_mod_lookup() runs the risk of
 calling crypto_larval_kill() on the same larval twice, which does not
 appear to be handled sanely.
 
 I can easily crash the kernel by forcing a synchronization point just
 before the return crypt_larval_add(...) call in
 crypto_larval_lookup(). Basically, I added this sloppy sync code (I
 feel like there must be a better way to do this):
 
 +static atomic_t global_sync = ATOMIC_INIT(0);
 ...
 struct crypto_alg *crypto_larval_lookup(const char *name, u32 type, u32 mask)
 ...
 +   if (strncmp(name, asdf, 4) == 0) {
 +   int value;
 +
 +   value = atomic_add_return(1, global_sync);
 +   if (value == 1) {
 +   /* I was first to stall here, wait for inc. */
 +   while (atomic_read(global_sync) != 2)
 +   cpu_relax();
 +   } else if (value == 2) {
 +   /* I was second to stall here, wait for dec. */
 +   while (atomic_read(global_sync) != 1)
 +   cpu_relax();
 +   } else {
 +   BUG();
 +   }
 +   atomic_dec(global_sync);
 +   }
 
 return crypto_larval_add(name, type, mask);
  }
 
 And then ran code from userspace that did, effectively:
 
 struct sockaddr_alg sa = {
 .salg_family = AF_ALG,
 .salg_type   = hash,
 };
 strcpy(sa.salg_name, argv[1]);
 
 fork();
 ...
 sds[0] = socket(AF_ALG, SOCK_SEQPACKET, 0);
 bind(sds[0], (struct sockaddr *) sa, sizeof(sa));
 
 And ran this as ./tickle asdf1 to generate the two threads trying to
 find an invalid alg. The race looks possible even with valid algs, but
 this was the fastest path I could see to tickle the issue.
 
 With added printks to the kernel, it was clear that crypto_larval_kill
 was being called twice on the same larval, and the list_del() call was
 doing bad things. When I fixed that, the refcnt bug became very
 obvious. Here's the change I made for crypto_larval_kill:
 
 @@ -161,7 +166,8 @@ void crypto_larval_kill(struct crypto_alg *alg)
 struct crypto_larval *larval = (void *)alg;
 
 down_write(crypto_alg_sem);
 -   list_del(alg-cra_list);
 +   if (!list_empty(alg-cra_list))
 +   list_del_init(alg-cra_list);
 up_write(crypto_alg_sem);
 complete_all(larval-completion);
 crypto_alg_put(alg);
 
 It seems that there are too many put calls (mixed between
 crypto_alg_put() and crypto_mod_put(), which also calls
 crypto_alg_put()). See this annotated portion of
 crypto_alg_mod_lookup:
 
 /* This can (correctly) return the same larval for two threads. */
 larval = crypto_larval_lookup(name, type, mask);
 if (IS_ERR(larval) || !crypto_is_larval(larval))
 return larval;
 
 ok = crypto_probing_notify(CRYPTO_MSG_ALG_REQUEST, larval);
 
 if (ok == NOTIFY_STOP)
 /* This calls crypto_mod_put(), but sometimes also get?? */
 alg = crypto_larval_wait(larval);
 else {
 /* This directly calls crypto_mod_put */
 crypto_mod_put(larval);
 alg = ERR_PTR(-ENOENT);
 }
 /* This calls crypto_alg_put */
 crypto_larval_kill(larval);
 return alg;
 
 In the two-thread situation, the first thread gets a larval with
 refcnt 2 via crypto_larval_add. (Why 2?) The next thread finds the
 larval via crypto_larval_add's call to __crypto_alg_lookup() and sees
 the ref bump to 3. While exiting crypto_alg_mod_lookup, each thread
 decrements the ref count twice.
 
 It seems to me like either each call to crypto_larval_lookup() should
 result in a refcount bumped by two, or crypto_alg_mod_lookup() should
 decrement only once, and the initial refcount should be 1 not 2 from
 crypto_larval_add. But it's not clear to me which is sensible here.
 
 What's the right solution here?
 
 Thanks,
 
 -Kees
 
 -- 
 Kees Cook
 Chrome OS Security
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More 

Re: [PATCH] [char] random: fix priming of last_data

[Neil Horman]

On Tue, Mar 19, 2013 at 12:18:09PM -0400, Jarod Wilson wrote:
 Commit ec8f02da9e added priming of last_data per fips requirements.
 Unfortuantely, it did so in a way that can lead to multiple threads all
 incrementing nbytes, but only one actually doing anything with the extra
 data, which leads to some fun  random corruption and panics.
 
 The fix is to simply do everything needed to prime last_data in a single
 shot, so there's no window for multiple cpus to increment nbytes -- in
 fact, we won't even increment or decrement nbytes anymore, we'll just
 extract the needed EXTRACT_BYTES one time per pool and then carry on with
 the normal routine.
 
 All these changes have been tested across multiple hosts and architectures
 where panics were previously encoutered. The code changes are are strictly
 limited to areas only touched when when booted in fips mode.
 
 This change should also go into 3.8-stable, to make the myriads of fips
 users on 3.8.x happy.
 
 Tested-by: Jan Stancek jstan...@redhat.com
 Tested-by: Jan Stodola jstod...@redhat.com
 CC: Herbert Xu herb...@gondor.apana.org.au
 CC: Neil Horman nhor...@tuxdriver.com
 CC: David S. Miller da...@davemloft.net
 CC: Matt Mackall m...@selenic.com
 CC: Theodore Ts'o ty...@mit.edu
 CC: linux-crypto@vger.kernel.org
 CC: sta...@vger.kernel.org
 Signed-off-by: Jarod Wilson ja...@redhat.com
 ---
  drivers/char/random.c |   30 +++---
  1 files changed, 15 insertions(+), 15 deletions(-)
 
 diff --git a/drivers/char/random.c b/drivers/char/random.c
 index 85e81ec..15e5a2b 100644
 --- a/drivers/char/random.c
 +++ b/drivers/char/random.c
 @@ -953,10 +953,23 @@ static ssize_t extract_entropy(struct entropy_store *r, 
 void *buf,
  {
   ssize_t ret = 0, i;
   __u8 tmp[EXTRACT_SIZE];
 + unsigned long flags;
  
   /* if last_data isn't primed, we need EXTRACT_SIZE extra bytes */
 - if (fips_enabled  !r-last_data_init)
 - nbytes += EXTRACT_SIZE;
 + if (fips_enabled) {
 + spin_lock_irqsave(r-lock, flags);
 + if (!r-last_data_init) {
 + r-last_data_init = true;
 + spin_unlock_irqrestore(r-lock, flags);
 + trace_extract_entropy(r-name, EXTRACT_SIZE,
 +   r-entropy_count, _RET_IP_);
 + xfer_secondary_pool(r, EXTRACT_SIZE);
 + extract_buf(r, tmp);
 + spin_lock_irqsave(r-lock, flags);
 + memcpy(r-last_data, tmp, EXTRACT_SIZE);
 + }
 + spin_unlock_irqrestore(r-lock, flags);
 + }
  
   trace_extract_entropy(r-name, nbytes, r-entropy_count, _RET_IP_);
   xfer_secondary_pool(r, nbytes);
 @@ -966,19 +979,6 @@ static ssize_t extract_entropy(struct entropy_store *r, 
 void *buf,
   extract_buf(r, tmp);
  
   if (fips_enabled) {
 - unsigned long flags;
 -
 -
 - /* prime last_data value if need be, per fips 140-2 */
 - if (!r-last_data_init) {
 - spin_lock_irqsave(r-lock, flags);
 - memcpy(r-last_data, tmp, EXTRACT_SIZE);
 - r-last_data_init = true;
 - nbytes -= EXTRACT_SIZE;
 - spin_unlock_irqrestore(r-lock, flags);
 - extract_buf(r, tmp);
 - }
 -
   spin_lock_irqsave(r-lock, flags);
   if (!memcmp(tmp, r-last_data, EXTRACT_SIZE))
   panic(Hardware RNG duplicated output!\n);
 -- 
 1.7.1
 
 
Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH v3] random: prime last_data value per fips requirements

[Neil Horman]

On Tue, Nov 06, 2012 at 10:42:42AM -0500, Jarod Wilson wrote:
 The value stored in last_data must be primed for FIPS 140-2 purposes. Upon
 first use, either on system startup or after an RNDCLEARPOOL ioctl, we
 need to take an initial random sample, store it internally in last_data,
 then pass along the value after that to the requester, so that consistency
 checks aren't being run against stale and possibly known data.
 
 v2: streamline code flow a bit, eliminating extra loop and spinlock in the
 case where we need to prime, and account for the extra primer bits.
 
 v3: extract_buf() can't be called with spinlock already held, so bring
 back some extra lock/unlock calls.
 
 CC: Herbert Xu herb...@gondor.apana.org.au
 CC: David S. Miller da...@davemloft.net
 CC: Neil Horman nhor...@tuxdriver.com
 CC: Matt Mackall m...@selenic.com
 CC: linux-crypto@vger.kernel.org
 Signed-off-by: Jarod Wilson ja...@redhat.com
 ---
  drivers/char/random.c |   17 +
  1 files changed, 17 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/char/random.c b/drivers/char/random.c
 index b86eae9..d0139df 100644
 --- a/drivers/char/random.c
 +++ b/drivers/char/random.c
 @@ -437,6 +437,7 @@ struct entropy_store {
   int entropy_count;
   int entropy_total;
   unsigned int initialized:1;
 + bool last_data_init;
   __u8 last_data[EXTRACT_SIZE];
  };
  
 @@ -957,6 +958,10 @@ static ssize_t extract_entropy(struct entropy_store *r, 
 void *buf,
   ssize_t ret = 0, i;
   __u8 tmp[EXTRACT_SIZE];
  
 + /* if last_data isn't primed, we need EXTRACT_SIZE extra bytes */
 + if (fips_enabled  !r-last_data_init)
 + nbytes += EXTRACT_SIZE;
 +
   trace_extract_entropy(r-name, nbytes, r-entropy_count, _RET_IP_);
   xfer_secondary_pool(r, nbytes);
   nbytes = account(r, nbytes, min, reserved);
 @@ -967,6 +972,17 @@ static ssize_t extract_entropy(struct entropy_store *r, 
 void *buf,
   if (fips_enabled) {
   unsigned long flags;
  
 +
 + /* prime last_data value if need be, per fips 140-2 */
 + if (!r-last_data_init) {
 + spin_lock_irqsave(r-lock, flags);
 + memcpy(r-last_data, tmp, EXTRACT_SIZE);
 + r-last_data_init = true;
 + nbytes -= EXTRACT_SIZE;
 + spin_unlock_irqrestore(r-lock, flags);
 + extract_buf(r, tmp);
 + }
 +
   spin_lock_irqsave(r-lock, flags);
   if (!memcmp(tmp, r-last_data, EXTRACT_SIZE))
   panic(Hardware RNG duplicated output!\n);
 @@ -1086,6 +1102,7 @@ static void init_std_data(struct entropy_store *r)
  
   r-entropy_count = 0;
   r-entropy_total = 0;
 + r-last_data_init = false;
   mix_pool_bytes(r, now, sizeof(now), NULL);
   for (i = r-poolinfo-POOLBYTES; i  0; i -= sizeof(rv)) {
   if (!arch_get_random_long(rv))
 -- 
 1.7.1
 
 
Thanks Jarod.
Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH] ansi_cprng: enforce key != seed in fips mode

[Neil Horman]

On Thu, Nov 03, 2011 at 04:24:45PM -0400, Jarod Wilson wrote:
 Apparently, NIST is tightening up its requirements for FIPS validation
 with respect to RNGs. Its always been required that in fips mode, the
 ansi cprng not be fed key and seed material that was identical, but
 they're now interpreting FIPS 140-2, section AS07.09 as requiring that
 the implementation itself must enforce the requirement. Easy fix, we
 just do a memcmp of key and seed in fips_cprng_reset and call it a day.
 
 CC: Neil Horman nhor...@tuxdriver.com
 CC: Stephan Mueller smuel...@atsec.com
 CC: Steve Grubb sgr...@redhat.com
 Signed-off-by: Jarod Wilson ja...@redhat.com
 ---
  crypto/ansi_cprng.c |5 +
  1 files changed, 5 insertions(+), 0 deletions(-)
 
 diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
 index ffa0245..a7fdcb4 100644
 --- a/crypto/ansi_cprng.c
 +++ b/crypto/ansi_cprng.c
 @@ -414,10 +414,15 @@ static int fips_cprng_get_random(struct crypto_rng 
 *tfm, u8 *rdata,
  static int fips_cprng_reset(struct crypto_rng *tfm, u8 *seed, unsigned int 
 slen)
  {
   u8 rdata[DEFAULT_BLK_SZ];
 + u8 *key = seed + DEFAULT_BLK_SZ;
   int rc;
  
   struct prng_context *prng = crypto_rng_ctx(tfm);
  
 + /* fips strictly requires seed != key */
 + if (!memcmp(seed, key, DEFAULT_PRNG_KSZ))
 + return -EINVAL;
 +
   rc = cprng_reset(tfm, seed, slen);
  
   if (!rc)
 -- 
 1.7.1
 
 
Thank you Jarod, The idea is fine to me.  Unfortunately, because you're indexing
into the seed to grab the key value, just like cprng_reset does now, you
probably need to add the slen checks that cprng_reset does to make sure theres
enough seed data as well, to avoid dereferencing unallocated memory.  If you fix
that up I'll ack it.

Neil

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

Re: [PATCH v2] ansi_cprng: enforce key != seed in fips mode

[Neil Horman]

On Fri, Nov 04, 2011 at 10:01:25AM -0400, Jarod Wilson wrote:
 Apparently, NIST is tightening up its requirements for FIPS validation
 with respect to RNGs. Its always been required that in fips mode, the
 ansi cprng not be fed key and seed material that was identical, but
 they're now interpreting FIPS 140-2, section AS07.09 as requiring that
 the implementation itself must enforce the requirement. Easy fix, we
 just do a memcmp of key and seed in fips_cprng_reset and call it a day.
 
 v2: Per Neil's advice, ensure slen is sufficiently long before we
 compare key and seed to avoid looking at potentially unallocated mem.
 
 CC: Neil Horman nhor...@tuxdriver.com
 CC: Stephan Mueller smuel...@atsec.com
 CC: Steve Grubb sgr...@redhat.com
 Signed-off-by: Jarod Wilson ja...@redhat.com
Thanks Jarod.  Adding Herbert to the cc list so he can pull this into the crypto
tree.

Acked-by: Neil Horman nhor...@tuxdriver.com

 ---
  crypto/ansi_cprng.c |8 
  1 files changed, 8 insertions(+), 0 deletions(-)
 
 diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
 index ffa0245..6ddd99e 100644
 --- a/crypto/ansi_cprng.c
 +++ b/crypto/ansi_cprng.c
 @@ -414,10 +414,18 @@ static int fips_cprng_get_random(struct crypto_rng 
 *tfm, u8 *rdata,
  static int fips_cprng_reset(struct crypto_rng *tfm, u8 *seed, unsigned int 
 slen)
  {
   u8 rdata[DEFAULT_BLK_SZ];
 + u8 *key = seed + DEFAULT_BLK_SZ;
   int rc;
  
   struct prng_context *prng = crypto_rng_ctx(tfm);
  
 + if (slen  DEFAULT_PRNG_KSZ + DEFAULT_BLK_SZ)
 + return -EINVAL;
 +
 + /* fips strictly requires seed != key */
 + if (!memcmp(seed, key, DEFAULT_PRNG_KSZ))
 + return -EINVAL;
 +
   rc = cprng_reset(tfm, seed, slen);
  
   if (!rc)
 -- 
 1.7.1
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] random: add blocking facility to urandom

[Neil Horman]

On Thu, Sep 08, 2011 at 08:41:57AM +0200, Tomas Mraz wrote:
 On Wed, 2011-09-07 at 19:57 -0400, Neil Horman wrote: 
  On Wed, Sep 07, 2011 at 04:56:49PM -0400, Steve Grubb wrote:
   On Wednesday, September 07, 2011 04:37:57 PM Sasha Levin wrote:
Anyway, it won't happen fast enough to actually not block.

Writing 1TB of urandom into a disk won't generate 1TB (or anything close
to that) of randomness to cover for itself.
   
   We don't need a 1:1 mapping of RNG used to entropy acquired. Its more on 
   the scale of 
   8,000,000:1 or higher.
   
  Where are you getting that number from?
  
  You may not need it, but there are other people using this facility as well 
  that
  you're not considering.  If you assume that in the example Sasha has given, 
  if
  conservatively, you have a modern disk with 4k sectors, and you fill each 4k
  sector with the value obtained from a 4 byte read from /dev/urandom, You 
  will:
  
  1) Generate an interrupt for every page you write, which in turn will add at
  most 12 bits to the entropy pool
  
  2) Extract 32 bits from the entropy pool
  
  Thats just a loosing proposition.   Barring further entropy generation from
  another source, this is bound to stall with this feature enabled. 
 Why so? In the case the blocking limit is on 8MBits of data read
 from /dev/urandom per every 1 bit added to the entropy pool (this is not
 the exact way how the patch behaves but we can approximate that) I do
 not see the /dev/urandom can block if the bytes read from it are written
Easy, all you have to do is read 8MB of data out of /dev/urandom (plus whatever
other conditions are needed to first drain the entropy pool), prior to that bit
of entropy getting added.

 to disk device - of course only if the device adds entropy into the
 primary pool when there are writes on the device.
Yes, and thats a problem.  We're assuming in the above case that writes to disk
generate interrupts which in turn generate entropy in the pool.  If that
happens, then yes, it can be difficult (though far from impossible) to block on
urandom with this patch and a sufficiently high blocking threshold.  But
interrupt randomness is only added for interrupts flagged with
IRQF_SAMPLE_RANDOM, and if you look, almost no hard irqs add that flag.  So its
possible (and even likely) that writing to disk will not generate additional
entropy.

 
 Of course you can still easily make the /dev/urandom to occasionally
 block with this patch, just read the data and drop it.
 
 But you have to understand that the value that will be set with the
 sysctl added by this patch will be large in the order of millions of
 bits.
 
You can guarantee that?  This sysctl allows for a setting of 2 just as easily as
it allows for a setting of 8,000,000.  And the former is sure to break or
otherwise adversely affect applications that expect urandom to never block.
Thats what Sasha was referring to, saying that patch makes it easy for admins to
make serious mistakes.

Neil

 -- 
 Tomas Mraz
 No matter how far down the wrong road you've gone, turn back.
   Turkish proverb
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] random: add blocking facility to urandom

[Neil Horman]

On Thu, Sep 08, 2011 at 09:11:12AM -0400, Steve Grubb wrote:
 On Thursday, September 08, 2011 08:52:34 AM Neil Horman wrote:
   to disk device - of course only if the device adds entropy into the
   primary pool when there are writes on the device.
  
  Yes, and thats a problem.  We're assuming in the above case that writes to
  disk generate interrupts which in turn generate entropy in the pool.  If
  that happens, then yes, it can be difficult (though far from impossible)
  to block on urandom with this patch and a sufficiently high blocking
  threshold.  But interrupt randomness is only added for interrupts flagged
  with
  IRQF_SAMPLE_RANDOM, and if you look, almost no hard irqs add that flag.  So
  its possible (and even likely) that writing to disk will not generate
  additional entropy.
 
 The system being low on entropy is another problem that should be addressed. 
 For our 
 purposes, we cannot say take it from TPM or RDRND or any plugin board. We 
 have to have 
 the mathematical analysis that goes with it, we need to know where the 
 entropy comes 
 from, and a worst case entropy estimation. It has to be documented in detail. 
 The only 
 way we can be certain is if its based on system events. Linux systems are 
 constantly 
 low on entropy and this really needs addressing. But that is a separate 
 issue. For 
 real world use, I'd recommend everyone use a TPM chip + rngd and you'll never 
 be short 
 on random numbers. But in the case where we are certifying the OS, we need 
 the 
 mathematical argument to prove that unaided, things are correct.
 
I agree, it would be great if we had more entropy as a rule, but thats not
really what this patch is about.  Its about how we behave in our various
interfaces when we don't have entropy.

  
   Of course you can still easily make the /dev/urandom to occasionally
   block with this patch, just read the data and drop it.
   
   But you have to understand that the value that will be set with the
   sysctl added by this patch will be large in the order of millions of
   bits.
  
  You can guarantee that?  
 
 One proposal I made to Jarod was to add some minimum threshold that would 
 prevent 
 people from setting a value of 2, for example. Maybe the threshold could be 
 set at 64K 
 or higher depending on what number we get back from BSI.
 
  This sysctl allows for a setting of 2 just as  easily as it allows for a 
  setting of
  8,000,000.  And the former is sure to break or otherwise adversely affect
  applications that expect urandom to never block. Thats what Sasha was 
  referring to,
  saying that patch makes it easy for admins to make serious mistakes.
 
 Would a sufficiently high threshold make this easier to accept?
 

I don't know, but IMO, no.  The problems with this implementation go beyond just
picking the appropriate threshold.  As several others have commented, theres
problems:

1) With having a threshold at all - I still don't think its clear what a 'good'
theshold is and why.  I've seen 8,000,000 bytes beyond zero entropy tossed
about.  I presume thats used because its been shown that after 8,000,000 bytes
read beyond zero entropy, the internal state of the urandom device can be
guessed?  If so, how?  If not, what the magic number?

2) With the implementation.  There are still unaddressed concerns about
applications which expect urandom to never block living in conjunction with
applications that can tolerate it.  As you noted above entropy is in short
supply in Linux systems.  Regardless of what threshold you set, its possible
that it will not be high enough to prevent urandom blocking for indefinate
periods of time.  Not addressing this I think is a complete show stopper.  The
CUSE driver has been proposed as a solution here and I think its a good one.  It
lets those that are worried about this sort of attack mitigate it and leaves the
rest of the world alone (and ostensibly is auditable)

Neil

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

Re: [PATCH] random: add blocking facility to urandom

[Neil Horman]

On Wed, Sep 07, 2011 at 04:02:24PM -0400, Steve Grubb wrote:
 On Wednesday, September 07, 2011 03:27:37 PM Ted Ts'o wrote:
  On Wed, Sep 07, 2011 at 02:26:35PM -0400, Jarod Wilson wrote:
   We're looking for a generic solution here that doesn't require
   re-educating every single piece of userspace. And anything done in
   userspace is going to be full of possible holes -- there needs to be
   something in place that actually *enforces* the policy, and
   centralized accounting/tracking, lest you wind up with multiple
   processes racing to grab the entropy.
  
  Yeah, but there are userspace programs that depend on urandom not
  blocking... so your proposed change would break them.
 
 The only time this kicks in is when a system is under attack. If you have set 
 this and 
 the system is running as normal, you will never notice it even there. Almost 
 all uses 
 of urandom grab 4 bytes and seed openssl or libgcrypt or nss. It then uses 
 those 
 libraries. There are the odd cases where something uses urandom to generate a 
 key or 
 otherwise grab a chunk of bytes, but these are still small reads in the 
 scheme of 
Theres no way you can guarantee that.  A quick lsof on my system here shows 27
unique pids that are holding /dev/urandom open, and while they may all be small 
reads,
taken in aggregate, I can imagine that they could pull a significant amount of
entropy out of /dev/urandom.

 things. Can you think of any legitimate use of urandom that grabs 100K or 1M 
 from 
 urandom? Even those numbers still won't hit the sysctl on a normally function 
 system.
 
How can you be sure of that?  This seems to make assumptions about both the rate
at which entropy is drained from /dev/urandom and the limit at which you will
start blocking, neither of which you can be sure of.

 When a system is underattack, do you really want to be using a PRNG for 
 anything like 
How can you be sure that this only happens when a system is under some sort of
attack.  /dev/urandom is there for user space to use, and we can't make
assumptions as to how it will get drawn from.  What if someone was running some
monte-carlo based test program?  That could completely exhaust the entropy in
/dev/urandom and would be perfectly legitimate.

 seeding openssl? Because a PRNG is what urandom degrades into when its 
 attacked. If 
 enough bytes are read that an attacker can guess the internal state of the 
 RNG, do you 
 really want it seeding a openssh session? At that point you really need it to 
 stop 
 momentarily until it gets fresh entropy so the internal state is unknown. 
 That's what 
 this is really about.
I never really want my ssh session to be be seeded with non-random data.  Of
course, in my mind thats an argument for making ssh use /dev/random rather than
/dev/urandom, but I'm willing to take the tradeoff in speed most of the time.

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

Re: [PATCH] random: add blocking facility to urandom

[Neil Horman]

On Wed, Sep 07, 2011 at 04:56:49PM -0400, Steve Grubb wrote:
 On Wednesday, September 07, 2011 04:37:57 PM Sasha Levin wrote:
  On Wed, 2011-09-07 at 16:30 -0400, Steve Grubb wrote:
   On Wednesday, September 07, 2011 04:23:13 PM Sasha Levin wrote:
On Wed, 2011-09-07 at 16:02 -0400, Steve Grubb wrote:
 On Wednesday, September 07, 2011 03:27:37 PM Ted Ts'o wrote:
  On Wed, Sep 07, 2011 at 02:26:35PM -0400, Jarod Wilson wrote:
   We're looking for a generic solution here that doesn't require
   re-educating every single piece of userspace. And anything done
   in userspace is going to be full of possible holes -- there
   needs to be something in place that actually *enforces* the
   policy, and centralized accounting/tracking, lest you wind up
   with multiple processes racing to grab the entropy.
  
  Yeah, but there are userspace programs that depend on urandom not
  blocking... so your proposed change would break them.
 
 The only time this kicks in is when a system is under attack. If you
 have set this and the system is running as normal, you will never
 notice it even there. Almost all uses of urandom grab 4 bytes and
 seed openssl or libgcrypt or nss. It then uses those libraries.
 There are the odd cases where something uses urandom to generate a
 key or otherwise grab a chunk of bytes, but these are still small
 reads in the scheme of things. Can you think of any legitimate use
 of urandom that grabs 100K or 1M from urandom? Even those numbers
 still won't hit the sysctl on a normally function system.

As far as I remember, several wipe utilities are using /dev/urandom to
overwrite disks (possibly several times).
   
   Which should generate disk activity and feed entropy to urandom.
  
  I thought you need to feed random, not urandom.
 
 I think they draw from the same pool.
 
They share the primary pool, where timer/interrupt/etc randomness is fed in.
/dev/random and /dev/urandom each have their own secondary pools however.

  
  Anyway, it won't happen fast enough to actually not block.
  
  Writing 1TB of urandom into a disk won't generate 1TB (or anything close
  to that) of randomness to cover for itself.
 
 We don't need a 1:1 mapping of RNG used to entropy acquired. Its more on the 
 scale of 
 8,000,000:1 or higher.
 
Where are you getting that number from?

You may not need it, but there are other people using this facility as well that
you're not considering.  If you assume that in the example Sasha has given, if
conservatively, you have a modern disk with 4k sectors, and you fill each 4k
sector with the value obtained from a 4 byte read from /dev/urandom, You will:

1) Generate an interrupt for every page you write, which in turn will add at
most 12 bits to the entropy pool

2) Extract 32 bits from the entropy pool

Thats just a loosing proposition.   Barring further entropy generation from
another source, this is bound to stall with this feature enabled. 


 
Something similar probably happens for getting junk on disks before
creating an encrypted filesystem on top of them.
   
   During system install, this sysctl is not likely to be applied.
  
  It may happen at any time you need to create a new filesystem, which
  won't necessarily happen during system install.
  
  See for example the instructions on how to set up a LUKS filesystem:
  https://wiki.archlinux.org/index.php/System_Encryption_with_LUKS#Preparatio
  n_and_mapping
 
 Those instructions might need to be changed. That is one way of many to get 
 random 
 numbers on the disk. Anyone really needing the security to have the sysctl on 
 will 
 also probably accept that its doing its job and keeping the numbers random. 
 Again, no 
 effect unless you turn it on.
 

And then its enforced on everyone, even those applications that don't want
it/can't work with it on.  This has to be done in such a way that its opt-in on
a per-application basis.  The CUSE idea put up previously sounds like a pretty
good way to do this.  The ioctl for per-fd blocking thresholds is another way to
go.

Neil

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

Re: [PATCH 0/5] Feed entropy pool via high-resolution clocksources

[Neil Horman]

On Sat, Jun 18, 2011 at 03:40:50PM -0700, H. Peter Anvin wrote:
 On 06/17/2011 01:28 PM, Matt Mackall wrote:
 
  The one use case that it is cryptographically insufficient for is to
  seed a new PRNG, which probably means it is unsuitable for being fed
  as-is into /dev/random.
  
  The thing to understand about the input side of /dev/random is that it's
  COMPLETELY immune to untrusted data. So there's absolutely no harm in
  sending it data of questionable entropy so long as you don't tell it to
  account it. And, of course, if it DOES contain entropy, it makes things
  better.
  
  Think of it this way: I have a coin in my pocket. You, the attacker,
  tell me to flip it. You can do that any number of times and not improve
  your guess about the coin's state over your initial guess. This is what
  it means to have a reversible mixing function: no number of iterations
  reduces the degrees of freedom of the pool.
  
 
 What I meant is that it is unsuitable to *bypass the pool* for
 /dev/random.  I think we can -- and almost certainly should -- use
 RDRAND on the input side; we just have to figure out the accounting.
 
From your description of the instruction, this sounds almost identical to me as
what I suggested in using an instance of the cprng module in the kernel as the
drng coupled with using the tsc entropy as the trng input to reseed it.  We
could use rnrand when possible on the input to the entropy pool, and the
software cprng when that wasn't available.

 However, RDRAND is high enough quality (and high enough bandwidth) that
 it should be not just possible but desirable to completely bypass the
 pool system for /dev/urandom users and pull straight from the RDRAND
 instruction.  I don't actually know what the exact numbers look like,
 but the stall conditions being looked at are of the order of every core
 in the socket trying to execute RDRAND at the same time.
 
It sounds to me like, if its desireous to bypass the entropy pool, then we
should bypass the /dev/random path altogether.  Why not write a hwrng driver
that can export access to the rdrand instruction via a misc device.

Neil

   -hpa
 
 -- 
 H. Peter Anvin, Intel Open Source Technology Center
 I work for Intel.  I don't speak on their behalf.
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Feed entropy pool via high-resolution clocksources

[Neil Horman]

On Fri, Jun 17, 2011 at 02:51:31PM -0400, Jarod Wilson wrote:
 Matt Mackall wrote:
 On Wed, 2011-06-15 at 10:49 -0400, Jarod Wilson wrote:
 Matt Mackall wrote:
 On Tue, 2011-06-14 at 18:51 -0400, Jarod Wilson wrote:
 Matt Mackall wrote:
 ...
 But that's not even the point. Entropy accounting here is about
 providing a theoretical level of security above cryptographically
 strong. As the source says:
 
 Even if it is possible to  analyze SHA in some clever way, as long as
 the amount of data returned from the generator is less than the inherent
 entropy in the pool, the output data is totally unpredictable.
 
 This is the goal of the code as it exists. And that goal depends on
 consistent _underestimates_ and accurate accounting.
 Okay, so as you noted, I was only crediting one bit of entropy per byte
 mixed in. Would there be some higher mixed-to-credited ratio that might
 be sufficient to meet the goal?
 As I've mentioned elsewhere, I think something around .08 bits per
 timestamp is probably a good target. That's the entropy content of a
 coin-flip that is biased to flip heads 99 times out of 100. But even
 that isn't good enough in the face of a 100Hz clock source.
 
 And obviously the current system doesn't handle fractional bits at all.
 What if only one bit every n samples were credited? So 1/n bits per
 timestamp, effectively, and for an n of 100, that would yield .01 bits
 per timestamp. Something like this:
 
 Something like that would work, sure. But it's a hack/abuse -relative
 to the current framework-. I'm reluctant to just pile on the hacks on
 the current system, as that just means getting it coherent is that much
 further away.
 
 Something I should have probably made clearer was that we were
 looking for a semi-quick improvement for a particular use case that
 involves a certain 2.6.32-based kernel... For the short term, we're
 looking at some userspace alternatives, such as a Linux
 implementation of an entropy seed approach BSI approves of.
 Reference doc here, but in German:
 
 https://www.bsi.bund.de/ContentBSI/Publikationen/TechnischeRichtlinien/tr02102/index_htm.html
 
 Longer-term, we're having some discussions related to the revamped
 framework you've laid out. I'll table this clocksource-based entropy
 contribution code for now.
 
 Also, thank you for putting up with me. ;)
 
Not to prolong this conversation, but Matt, since you seem talkative about it,
has any consideration been given to using a periodically reseeded DRNG as an
input to the entropy pool?  some of the papers that Jarod referenced in this
thread suggest that using the tsc entropy as a reseed factor to a drng can
provide a good non-predictable source of entropy.

Thoughts?
Neil

 -- 
 Jarod Wilson
 ja...@redhat.com
 
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add RNG support to AF_ALG (v2)

[Neil Horman]

On Fri, Jan 21, 2011 at 05:00:05PM +1100, Herbert Xu wrote:
 On Thu, Jan 20, 2011 at 06:34:40PM -0500, Neil Horman wrote:
 
  Herbert, Sorry to bug you about this, but are you still planning on pulling 
  this
  now that Linus has the infrastructure scheduled for 2.6.38?
 
 I think it's best if we leave this out for now, unless we can
 come up with some way of merging this with the hardware RNG
 interface so that we are not duplicating an existing user interface.
 
I take your point, but I'm not certain I agree that we are duplicating an
existing user interface.  A cursory glance would say that we are, but
/dev/random and /dev/urandom really just provide access to the kernels entropy
pools, whereas the AF_ALG provides access to instances of any RNG the kernel has
to offer as well as the key management services that AF_ALG has, which I think
is adventageous, given that the CPRNG requires keying to work properly.

 Could we perhaps merge this with the HWRNG interface somehow?
We could certainly, add an ioctl to place /dev/[u]random in a cprng mode, and
another to allow key setting/resets/etc, but that seems fairly limiting in that
only one instace of a cprng could be accessed at a time.

 Or maybe we should just expose ansi_cprng (I presume you only
 need that) through the hwrng interface?
 
Again, we could, but that doesn't seem wise if:
1) A user is expecting truly random data
2) A user needs to truly have predicitbility of their random number set (if
they're using the cprng, multiple un-cordinated users breaks the ability to
predict the rng data).

Another alternative (just off the top of my head), might be to eliminate the
hwrng interface in the kernel entirely, and replace it with the AF_ALG based
interface.  People requiring access to /dev/random until their applications can
be migrated could still access /dev/[u]random via a user space daemon that opens
2 unix sockets, binds them to /dev/[u]random, and proxies them to two AF_ALG
sockets connected to the blocking and non-blocking entropy pools in the kernel.

Not saying thats a great idea mind, just brainstorming ways we can eliminate
interface duplication without restricting the CPRNG to a character based
interface, when AF_ALG provides it so much more.
Neil

 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
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add RNG support to AF_ALG (v2)

[Neil Horman]

On Fri, Jan 21, 2011 at 11:35:17PM +1100, Herbert Xu wrote:
 On Fri, Jan 21, 2011 at 07:09:50AM -0500, Neil Horman wrote:
 
  I take your point, but I'm not certain I agree that we are duplicating an
  existing user interface.  A cursory glance would say that we are, but
  /dev/random and /dev/urandom really just provide access to the kernels 
  entropy
  pools, whereas the AF_ALG provides access to instances of any RNG the 
  kernel has
  to offer as well as the key management services that AF_ALG has, which I 
  think
  is adventageous, given that the CPRNG requires keying to work properly.
 
 I'm not talking about /dev/random, I'm talking the hwrng interface
 in drivers/char/hwrng.
 
Doh!  I'm sorry, that was stupid of me.  Yes that makes alot more sense.  I'll
try another pass, adding the cprng to that interface instead.

Thanks!
Neil

 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
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add RNG support to AF_ALG (v2)

[Neil Horman]

On Mon, Dec 13, 2010 at 04:25:14PM -0500, Neil Horman wrote:
 Change notes:
 Changed rng_rcvmsg to allocate a fixed size maximum temp block to store rng 
 data
 when recvmsg is called.  This should prevent malicious DoS from user space by
 tring to receive obscene amounts of random data in one call.  Instead now we
 loop using the same block of data and copy it incrementally to the user space
 buffer using memcpy_toiovecend
 
 Also changed the accept routine to only allocate a new rng, and not store the
 seed value separately, simplifying the code somewhat.  also now we memset the
 parent sockets seed value to zero on free to hide the seed from intruders.
 
 Summary:
 This patch enhances the AF_ALG protocol family to include support for random
 number generator algorithms.  With this enhancment, users of the AF_ALG 
 protocol
 can now bind sockets to instances of the various RNG algorithms available to 
 the
 kernel.  For those RNG's that support it, instances can be reseeded using the
 SETKEY socket option within the AF_ALG socket family.  Like with hashes and
 ciphers, only the intially created socket allows seeding, and only child 
 sockets
 retured via accept may return random data.  Sending data on RNG instances is
 prohibited, only receiving RNG data is possible.
 
 Tested successfully using NIST provided RNG vectors by myself:
 Signed-off-by: Neil Horman nhor...@tuxdriver.com
 CC: Herbert Xu herb...@gondor.apana.org.au
 CC: David S. Miller da...@davemloft.net
Herbert, Sorry to bug you about this, but are you still planning on pulling this
now that Linus has the infrastructure scheduled for 2.6.38?

Neil

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

Re: Crypto Update for 2.6.38

[Neil Horman]

On Sat, Jan 08, 2011 at 03:23:04PM +0200, Nikos Mavrogiannopoulos wrote:
 On Fri, Jan 7, 2011 at 2:04 PM, Neil Horman nhor...@tuxdriver.com wrote:
 
  Btw, it doesn't have to be about performance per se. Does this allow
  people to use keys without actually _seeing_ those keys? Your example
  implies that that is not the case, but that's actually one of the few
  reasons to actually support a kernel crypto interface - the ability to
  have private personal keys around, but not having to actually let
  possibly untrusted programs see them.
  This actually is an indirect feature of this interface.  Using it, you can 
  open
  a algorithm socket, select a specific alg, assign a key, and then pass that
  socket descriptor over a unix socket to an another process using an
  SCM_RIGHTS ancilliary message.  The receiving process can then use children
  acceppted from that passed socket to preform the configured crypto operation
  without any knoweldge of the keys used in it.  I can write a demo app if you
  like.
 
 Several things have to be considered when extending an interface like
 that. For example, do the algorithm implementations protect against
 timing attacks, or keys can be recovered, using them? What is the
No, the kernel does not implement any protection against timing attacks in the
algorithms per-se, but preforming a timing attack against a kernel crypto
operation is going to be near impossible anyway, as precise timing measurements
are going to get obscured by interupts, scheduling jitter, lock contention, and
various other factors that will make measuring syscall time fairly useless.

 purpose of cryptographic key separation? If long term keys are to be
My only purpose was to answer Linus' question.  He wondered if other user space
programs could use instances of cyrpto algs over this interface without needing
to hold key data.  I was illustrating how that could be done.

Neil

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

Re: Crypto Update for 2.6.38

[Neil Horman]

On Thu, Jan 06, 2011 at 02:13:17PM -0800, Linus Torvalds wrote:
 On Thu, Jan 6, 2011 at 1:39 PM, Herbert Xu herb...@gondor.hengli.com.au 
 wrote:
  On Thu, Jan 06, 2011 at 01:23:19PM -0800, Linus Torvalds wrote:
 
  Explanations of interface. Code. Who uses it? What are the actual
  performance benefits on real code?
 
  You snipped out the bit in my reply where I expanded on it:
 
 You didn't expand on it AT ALL.
 
 You just mentioned the interface. I haven't seen WHAT THAT INTERFACE IS!
 
 How hard is that to understand?
 
  Here is the original cover email for the patches:
 
 Ok, this is more like it. This is roughly what I wanted to see:
 
  : Here is a sample hash program (note that these only illustrate
  : what the interface looks like and are not meant to be good examples
  : of coding :)
 
 But I'm still missing the part where you show that there is any actual
 use case that makes sense, and that actually improves performance.
 Maybe it's been posted somewhere else, but the thing is, you're asking
 _me_ to pull, and as a result you need to convince _me_ that this is a
 good idea. So if it's been posted/discussed extensively elsewhere,
 please point to those discussions.
 
 I really don't like adding interfaces that don't have hard uses
 associated with them. We've done it in the past, and it tends to be a
 morass and a bad idea. That's been true even when the idea has been my
 own, and thus obviously genius-level and clearly the RightThing(tm),
 like splice(). And it's why I push back on new interfaces when I see
 them.
 
 Btw, it doesn't have to be about performance per se. Does this allow
 people to use keys without actually _seeing_ those keys? Your example
 implies that that is not the case, but that's actually one of the few
 reasons to actually support a kernel crypto interface - the ability to
 have private personal keys around, but not having to actually let
 possibly untrusted programs see them.
 
This actually is an indirect feature of this interface.  Using it, you can open
a algorithm socket, select a specific alg, assign a key, and then pass that
socket descriptor over a unix socket to an another process using an
SCM_RIGHTS ancilliary message.  The receiving process can then use children
acceppted from that passed socket to preform the configured crypto operation
without any knoweldge of the keys used in it.  I can write a demo app if you
like.

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

[PATCH] Add RNG support to AF_ALG

[Neil Horman]

This patch enhances the AF_ALG protocol family to include support for random
number generator algorithms.  With this enhancment, users of the AF_ALG protocol
can now bind sockets to instances of the various RNG algorithms available to the
kernel.  For those RNG's that support it, instances can be reseeded using the
SETKEY socket option within the AF_ALG socket family.  Like with hashes and
ciphers, only the intially created socket allows seeding, and only child sockets
retured via accept may return random data.  Sending data on RNG instances is
prohibited, only receiving RNG data is possible.

Tested successfully using NIST provided RNG vectors by myself:
Signed-off-by: Neil Horman nhor...@tuxdriver.com
CC: Herbert Xu herb...@gondor.apana.org.au
CC: David S. Miller da...@davemloft.net
---
 crypto/Kconfig |9 +++
 crypto/Makefile|1 +
 crypto/algif_rng.c |  197 
 3 files changed, 207 insertions(+), 0 deletions(-)
 create mode 100644 crypto/algif_rng.c

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 96b0e55..ea448ca 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -864,6 +864,15 @@ config CRYPTO_USER_API_SKCIPHER
  This option enables the user-spaces interface for symmetric
  key cipher algorithms.
 
+config CRYPTO_USER_API_RNG
+   tristate User-space interface for Random Number Generator algorithms
+   depends on NET
+   select CRYPTO_ANSI_CPRNG
+   select CRYPTO_USER_API
+   help
+ This option enables the user-space interface for random number
+ generation algorithms.
+
 source drivers/crypto/Kconfig
 
 endif  # if CRYPTO
diff --git a/crypto/Makefile b/crypto/Makefile
index e9a399c..8868555 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_CRYPTO_GHASH) += ghash-generic.o
 obj-$(CONFIG_CRYPTO_USER_API) += af_alg.o
 obj-$(CONFIG_CRYPTO_USER_API_HASH) += algif_hash.o
 obj-$(CONFIG_CRYPTO_USER_API_SKCIPHER) += algif_skcipher.o
+obj-$(CONFIG_CRYPTO_USER_API_RNG) += algif_rng.o
 
 #
 # generic algorithms and the async_tx api
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
new file mode 100644
index 000..b660a2a
--- /dev/null
+++ b/crypto/algif_rng.c
@@ -0,0 +1,197 @@
+/*
+ * algif_rng: User-space interface for rng algorithms
+ *
+ * This file provides the user-space API for rng algorithms.
+ *
+ * Copyright (c) 2010 Neil Horman nhor...@tuxdriver.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ */
+#include crypto/rng.h
+#include crypto/if_alg.h
+#include linux/init.h
+#include linux/kernel.h
+#include linux/mm.h
+#include linux/module.h
+#include linux/net.h
+#include net/sock.h
+
+struct rng_ctx {
+   struct crypto_rng *ctx;
+   u8 *seed;
+   size_t seedlen;
+   char *name;
+   u32 type;
+   u32 mask;
+};
+
+static int rng_recvmsg(struct kiocb *unused, struct socket *sock,
+   struct msghdr *msg, size_t len, int flags)
+{
+   struct sock *sk = sock-sk;
+   struct alg_sock *ask = alg_sk(sk);
+   struct rng_ctx *ctx = ask-private;
+   int rc, i;
+   u8 *data = kzalloc(len, GFP_KERNEL);
+
+   if (!data)
+   return -ENOMEM;
+   rc = crypto_rng_get_bytes(ctx-ctx, data, len);
+   if (rc  0)
+   goto out;
+
+   rc = memcpy_toiovec(msg-msg_iov, data, len);
+out:
+   kfree(data);
+   return rc ?: len;
+}
+
+
+static const struct proto_ops algif_rng_ops = {
+   .family =   PF_ALG,
+
+   .connect=   sock_no_connect,
+   .socketpair =   sock_no_socketpair,
+   .getname=   sock_no_getname,
+   .ioctl  =   sock_no_ioctl,
+   .listen =   sock_no_listen,
+   .shutdown   =   sock_no_shutdown,
+   .getsockopt =   sock_no_getsockopt,
+   .mmap   =   sock_no_mmap,
+   .bind   =   sock_no_bind,
+   .setsockopt =   sock_no_setsockopt,
+   .poll   =   sock_no_poll,
+
+   .release=   af_alg_release,
+   .sendmsg=   sock_no_sendmsg,
+   .sendpage   =   sock_no_sendpage,
+   .recvmsg=   rng_recvmsg,
+   .accept =   sock_no_accept,
+};
+
+static void *rng_bind(const char *name, u32 type, u32 mask)
+{
+   struct rng_ctx *new = kzalloc(sizeof(struct rng_ctx), GFP_KERNEL);
+
+   if (!new)
+   goto out;
+
+   new-name = kmemdup(name, strnlen(name, 63)+1, GFP_KERNEL);
+   new-type = type;
+   new-mask = mask;
+out:
+   return new;
+}
+
+static void rng_release(void *private)
+{
+   struct rng_ctx *ctx = private;
+   crypto_free_rng(ctx-ctx);
+   kfree(ctx

Re: [PATCH] Add RNG support to AF_ALG

[Neil Horman]

On Mon, Dec 13, 2010 at 12:24:34PM -0500, Miloslav Trmac wrote:
 - Neil Horman nhor...@tuxdriver.com wrote:
  +static int rng_recvmsg(struct kiocb *unused, struct socket *sock,
  +   struct msghdr *msg, size_t len, int flags)
  +{
  +   struct sock *sk = sock-sk;
  +   struct alg_sock *ask = alg_sk(sk);
  +   struct rng_ctx *ctx = ask-private;
  +   int rc, i;
  +   u8 *data = kzalloc(len, GFP_KERNEL);
 There probably should be an upper limit on the allocation - perhaps just 
 always allocate a single page.
 
I'd rather allocate the explicit amount needed, just to avoid added memory
pressure allocating memory that we won't use, but yes, I can definately add an
upper limit to how much data can be requested in a single call.

  +static void rng_release(void *private)
  +{
  +   struct rng_ctx *ctx = private;
  +   crypto_free_rng(ctx-ctx);
  +   kfree(ctx-seed);
 Is a seed secret enough that it should be zeroed before freeing?  (Same in 
 setkey, accept_parent).
 
I don't think that nececcecary, strictly speaking, but it couldn't hurt.
Actually looking at it, I don't really need to duplicate the seed at all in
accept_parent.  I can probaby shrink that down considerably. 

Thanks for the notes Mirek, I'll post an updated version shortly.
Neil

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

[PATCH] Add RNG support to AF_ALG (v2)

[Neil Horman]

Change notes:
Changed rng_rcvmsg to allocate a fixed size maximum temp block to store rng data
when recvmsg is called.  This should prevent malicious DoS from user space by
tring to receive obscene amounts of random data in one call.  Instead now we
loop using the same block of data and copy it incrementally to the user space
buffer using memcpy_toiovecend

Also changed the accept routine to only allocate a new rng, and not store the
seed value separately, simplifying the code somewhat.  also now we memset the
parent sockets seed value to zero on free to hide the seed from intruders.

Summary:
This patch enhances the AF_ALG protocol family to include support for random
number generator algorithms.  With this enhancment, users of the AF_ALG protocol
can now bind sockets to instances of the various RNG algorithms available to the
kernel.  For those RNG's that support it, instances can be reseeded using the
SETKEY socket option within the AF_ALG socket family.  Like with hashes and
ciphers, only the intially created socket allows seeding, and only child sockets
retured via accept may return random data.  Sending data on RNG instances is
prohibited, only receiving RNG data is possible.

Tested successfully using NIST provided RNG vectors by myself:
Signed-off-by: Neil Horman nhor...@tuxdriver.com
CC: Herbert Xu herb...@gondor.apana.org.au
CC: David S. Miller da...@davemloft.net
---
 crypto/Kconfig |9 ++
 crypto/Makefile|1 +
 crypto/algif_rng.c |  212 
 3 files changed, 222 insertions(+), 0 deletions(-)
 create mode 100644 crypto/algif_rng.c

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 96b0e55..ea448ca 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -864,6 +864,15 @@ config CRYPTO_USER_API_SKCIPHER
  This option enables the user-spaces interface for symmetric
  key cipher algorithms.
 
+config CRYPTO_USER_API_RNG
+   tristate User-space interface for Random Number Generator algorithms
+   depends on NET
+   select CRYPTO_ANSI_CPRNG
+   select CRYPTO_USER_API
+   help
+ This option enables the user-space interface for random number
+ generation algorithms.
+
 source drivers/crypto/Kconfig
 
 endif  # if CRYPTO
diff --git a/crypto/Makefile b/crypto/Makefile
index e9a399c..8868555 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_CRYPTO_GHASH) += ghash-generic.o
 obj-$(CONFIG_CRYPTO_USER_API) += af_alg.o
 obj-$(CONFIG_CRYPTO_USER_API_HASH) += algif_hash.o
 obj-$(CONFIG_CRYPTO_USER_API_SKCIPHER) += algif_skcipher.o
+obj-$(CONFIG_CRYPTO_USER_API_RNG) += algif_rng.o
 
 #
 # generic algorithms and the async_tx api
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
new file mode 100644
index 000..8664793
--- /dev/null
+++ b/crypto/algif_rng.c
@@ -0,0 +1,212 @@
+/*
+ * algif_rng: User-space interface for rng algorithms
+ *
+ * This file provides the user-space API for rng algorithms.
+ *
+ * Copyright (c) 2010 Neil Horman nhor...@tuxdriver.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ */
+#include crypto/rng.h
+#include crypto/if_alg.h
+#include linux/init.h
+#include linux/kernel.h
+#include linux/mm.h
+#include linux/module.h
+#include linux/net.h
+#include net/sock.h
+
+struct rng_ctx {
+   struct crypto_rng *ctx;
+   u8 *seed;
+   size_t seedlen;
+   char *name;
+   u32 type;
+   u32 mask;
+};
+
+static int rng_recvmsg(struct kiocb *unused, struct socket *sock,
+   struct msghdr *msg, size_t len, int flags)
+{
+   struct sock *sk = sock-sk;
+   struct alg_sock *ask = alg_sk(sk);
+   struct crypto_rng *ctx = ask-private;
+   int rc = 0;
+   int rc2;
+   u8 *data;
+   size_t alen, clen;
+
+   /*
+* Prevent huge memory allocations from user space
+*/
+   alen = (len  PAGE_SIZE) ? PAGE_SIZE : len;
+   data = kzalloc(alen, GFP_KERNEL);
+
+   if (!data)
+   return -ENOMEM;
+
+   clen = 0;
+   while (clen != len) {
+   rc = crypto_rng_get_bytes(ctx, data, alen);
+   if (rc  0)
+   goto out;
+
+   rc2 = memcpy_toiovecend(msg-msg_iov, data, clen, rc);
+   if (rc2) {
+   rc = rc2;
+   goto out;
+   }
+
+   clen += rc;
+
+   /*
+* Get the remaining bytes
+*/
+   if ((len-clen)  alen)
+   alen = len-clen;
+   rc = 0;
+   }
+out:
+   kfree(data);
+   return rc ?: len;
+}
+
+
+static struct proto_ops algif_rng_ops = {
+   .family =   PF_ALG,
+
+   .connect

Re: [CRYPTO] obfuscating kernel pointers

[Neil Horman]

On Mon, Nov 15, 2010 at 09:43:12AM +0100, Tomas Mraz wrote:
 On Fri, 2010-11-12 at 08:32 -0500, Dan Rosenberg wrote: 
  Hi Crypto people,
  
  I'm planning on submitting a patch that introduces a new %p format
  specifier that obfuscates kernel pointers depending on privileges.  This
  change is for security reasons - many networking protocols expose
  pointers to socket structures in their /proc interfaces, which are
  attractive targets when exploiting other issues.
  
  It's been suggested that I initialize a secret value at boot, and use
  that as the key to a crypto hash function.  I should use a function that
  is relatively fast (ideally), produces a unique output based on its
  input of a pointer, and produces consistent output when given the same
  input.  It should be difficult to infer the input given only the output.
  
  I have two questions:
  
  1. What is a proper, safe way of initializing a random value at boot?
  Are there any existing examples that do this?
  
  2. Can you recommend a crypto algorithm that would be well suited for
  this pointer obfuscation?
 This would not be a 'hashing' algorithm but a simple block encryption
 algorithm in the ECB mode with the random key initialized at boot. The
 problem here is that the standard block ciphers have at least 64 bit
 block length as smaller block length ciphers would not be secure for
 general purpose uses. You would have to take an existing block cipher
 algorithm and modify it to achieve the smaller block length.
 
 -- 
 Tomas Mraz
 No matter how far down the wrong road you've gone, turn back.
   Turkish proverb
 
Something else occurs to me.  If this is to be implemented as an internal
modification to the %p format token, you're going to have to be careful as to
when you're able to use it.  Crypto algorithms can be built as individual
modules, and the api has them auto-load on demand when instances of them are
requested.  How are you going to be certain that the hash that you choose is
available?  For that matter, how are you going to know when its safe to ask for
the algorithm?  If you do a printk(...%p\n) early during the boot, you'll be
calling request_moule-...-call_usermodehelper_exec before the workqueue or any
of its datastructures are initalized, and that will oops you.

Neil

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

Re: [CRYPTO] obfuscating kernel pointers

[Neil Horman]

On Fri, Nov 12, 2010 at 12:39:41PM -0500, Dan Rosenberg wrote:
 Thanks for your response.
 
   
  Just use get_random_bytes, or initalize an instance of cprng with
  get_random_bytes.
  
 
 Will do.
 
  
  Depends on your goal, if you just wnat to hide the pointers, why not just 
  print
  NULL instead of the value?  If you want to maintain some level of 
  uniqueness,
  just pull sizeof (void *) random bytes from whatever method above and add 
  it to
  the pointer in question, and hope for the best.
  
 
 Unfortunately, neither of these sound like an option.  It's been
 requested from the networking folks that any replacement value for the
 socket addresses be a consistent unique identifier for object tracking
 purposes.  The current plan is to expose the real address to privileged
 readers, and expose a consistent obfuscated address that's only useful
 for tracking to unprivileged readers.
 
adding a consistent random value to a your void * pointers sounds like a fine
solution to the problem, then.  As long as you use the same random value for the
lifetime of the system, that will give you consistent values.  And you have to
use the same random input consistently to have consistent output on multiple
concatinations of the same file anyway.  This also has the advantage of not
having to do a crypto operation for every print/seq_sprintf/etc that contains a
%p.


  Honestly, though, I'm having trouble seeing the value of this.  What 
  interface in proc
  are you seeing that exposes pointers from kernel space in any meaningful 
  way?
  and if those cases exist, isn't selinux the solution to preventing exposure 
  of
  these values to processes without sufficient privlidges?
  Neil
  
 
 Lots of packet families expose them...see, for
 example, /proc/net/{tcp,udp,raw,unix}.  Since socket structures have
 function pointers, they are an appealing target in the event of a kernel
 memory write vulnerability.  The goal here is to make exploitation of
 such issues more difficult, including for distros that don't use
 SELinux.
 
huh, I guess they do spit out pointer values.  At any rate, I'm still having a
hard time seeing the value here.  If a distro doesn't enforce a security policy
sufficient to protect against information leaks from exposed files, I don't see
why the kernel should do that work for it.  But regardless, I suppose thats a
debate to hav when you post a patch :)

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

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Tue, Aug 10, 2010 at 10:06:05PM -0400, Miloslav Trmac wrote:
 - Neil Horman nhor...@redhat.com wrote:
  Ok, well, I suppose we're just not going to agree on this.  I don't know how
  else to argue my case, you seem to be bent on re-inventing the wheel 
  instead of
  using what we have.  Good luck. 
 Well, I basically spent yesterday learning about netlink and looking how it 
 can or can not be adapted.  I still see drawbacks that are not balanced by 
 any benefits that are exclusive to netlink.
 
 As a very unscientific benchmark, I modified a simple example program to add 
 a simple non-blocking getmsg(..., MSG_PEEK) call on a netlink socket after 
 each encryption operation; this is only a lower bound on the overhead because 
 no copying of data between userspace and the skbuffer takes place and zero 
 copy is still available.  With cbc(aes), encrypting 256 bytes per operation, 
 the throughput dropped from 156 MB/s to 124 MB/s (-20%); even with 32 KB per 
 operation there is still a decrease from 131 to 127 MB/s (-2.7%).
 
 If I have to do a little more programming work to get a permanent 20% 
 performance boost, why not?
 
Because your test is misleading.  By my read, all you did was add an extra
syscall to the work your already doing.  The best case result in such a
test is equivalent performance if the additional syscall takes near-zero time.
The test fails to take into account the change in programming model that you can
use in the kernel when you make the operation asynchronous.  What happens to
your test if you change the cipher your using to an asynchronous form
(ablkcipher or ahash)?  When you do that, you no longer need to stall the send
routine while the crypto operation completes.

I'm not saying that 2 syscalls will be faster than 1, but its not the slam dunk
you're indicating here.

 
 How about this: The existing ioctl (1-syscall interface) remains, using a 
 very minimal fixed header (probably just a handle and perhaps algorithm ID) 
 and using the netlink struct nlattr for all other attributes (both input and 
 output, although I don't expect many output attribute).
 
 - This gives us exactly the same flexibility benefits as using netlink 
 directly.
 - It uses the 1-syscall, higher performance, interface.
 - The crypto operations are run in process context, making it possible to 
 implement zero-copy operations and reliable auditing.
 - The existing netlink parsing routines (both in the kernel and in libnl) can 
 be used; formatting routines will have to be rewritten, but that's the easier 
 part.
 
This would be better, but it really just seems like you're re-inventing the
wheel at this point.  As noted above, I think your performance comparison fails
to account for advantages that can be leveraged in an asynchronous model.  The
zero-copy argument is misleading, as both a single syscall and a multiple
syscall are not zero copy, a copy_from_user and copy_to_user is required in
both cases.  About the only clear advantage that I see to using a single syscall
inteface is the additional audit information that you are afforded.  And I'm
still not sure if the additional audit information is required by FIPS or Common
Criteria.


Also, now that I'm poking about in it, how do you intend to support the async
interfaces?  aead/ablkcipher/ahash all require callbacks to be set when the
crypto operation completes.  I assume that, in the kernel your cryptodev code,
if it used the 1 syscall interface would setup a lock, and block until the
operation was complete?  If thats the case, and the actual crypto operation were
handled by an alternate task (see the cryptd module), wouldn't you be loosing
soe modicum of audit information as well, just as you would using the netlink
interface?
Neil

 This kind of partial netlink reuse already has a precedent in the kernel, see 
 zlib_compress_setup().
 
 Would everyone be happy with such an approach?
 Mirek
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Mon, Aug 09, 2010 at 08:00:55PM -0400, Miloslav Trmac wrote:
 - Herbert Xu herb...@gondor.hengli.com.au wrote:
 
  On Thu, Aug 05, 2010 at 10:17:53PM +0200, Miloslav Trmač wrote:
   Hello,
   following is a patchset providing an user-space interface to the kernel 
   crypto
   API.  It is based on the older, BSD-compatible, implementation, but the
   user-space interface is different.
  
  Thanks for the patches.
  
  Unfortunately it fails to satisfy the requirement of supporting
  all our existing kernel crypto interfaces, such as AEAD, as well
  as being flexible enough in adding new interfaces such as xor.
 Thanks for the review.
 
 I think I can add AEAD (and compression/RNG, if requested) easily enough, 
 I'll send an updated patch set.
 
 (Talking specifically about xor, xor does not seem to be cryptographic 
 operation that should be exposed as such to userspace - and AFAICS it is not 
 integrated in the crypto API algorithm mechanism in the kernel either.)
 
 Is the proposed interface acceptable in the general approach (enums for 
 algorithms/operations, unions for parameters, session init/update/finalize)?  
 With respect to flexibility, do you have specific suggestions or areas of 
 concern?

I know we spoke about this previously, but since you're asking publically, I'll
make my argument here so that we can have the debate in public as well.  I'm ok
wih the general enumeration of operations in user space (I honestly can't see
how you would do it otherwise).  What worries me though is the use ioctl for
these operations  and the flexibility that it denies you in future updates.

Specifically, my concerns are twofold:

1) struct format.  By passing down a structure as your doing through an ioctl
call, theres  no way to extend/modify that structure easily for future use.  For
instance the integration of aead might I think requires a blocking factor to be
sepcified, and entry for which you have no available field in your crypt_ops
structure.  If you extend the structure in a later release of this code, you
create a potential incompatibility with user space because you are no longer
guaranteed that the size of the crypt_op structure is the same, and you need to
be prepared for a range of sizes to get passed down, at which point it becomes
difficult to differentiate between older code thats properly formatted, and
newer code thats legitimately broken.  You might could add a version field to
mitigate that, but it gets ugly pretty quickly.

2) Use of pointers.  Thats just a pain.  You have the compat ioctl stuff in
place to handle the 32/64 bit conversions, but it would be really nice if you
could avoid having to maintain that extra code path.

Thats why I had suggested the use of a netlink protocol to manage this kind of
interface.  There are other issue to manage there, but they're really not that
big a deal, comparatively speaking, and that interface allows for the easy
specification of arbitrary length data in a fashion that doesn't cause user
space breakage when additional algorithms/apis are added.

Thats just my opinion, I'm sure others have differing views.  I'd be interested
to hear what others think.

Regards
Neil

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

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Tue, Aug 10, 2010 at 09:24:31AM -0400, Steve Grubb wrote:
 On Tuesday, August 10, 2010 08:46:28 am Neil Horman wrote:
  Specifically, my concerns are twofold:
  
  1) struct format.  By passing down a structure as your doing through an
  ioctl call, theres  no way to extend/modify that structure easily for
  future use.  For instance the integration of aead might I think requires a
  blocking factor to be sepcified, and entry for which you have no available
  field in your crypt_ops structure.  If you extend the structure in a later
  release of this code, you create a potential incompatibility with user
  space because you are no longer guaranteed that the size of the crypt_op
  structure is the same, and you need to be prepared for a range of sizes to
  get passed down, at which point it becomes difficult to differentiate
  between older code thats properly formatted, and newer code thats
  legitimately broken.  You might could add a version field to mitigate
  that, but it gets ugly pretty quickly.
 
 Yeah, this does call out for versioning. But the ioctls are just for crypto 
 parameter setup. Hopefully, that doesn't change too much since its just to 
 select an algorithm, possibly ask for a key to be wrapped and loaded, or ask 
 for a key to be created and exported. After setup, you just start using the 
 device.
  
 
  Thats why I had suggested the use of a netlink protocol to manage this kind
  of interface.  There are other issue to manage there, but they're really
  not that big a deal, comparatively speaking, and that interface allows for
  the easy specification of arbitrary length data in a fashion that doesn't
  cause user space breakage when additional algorithms/apis are added.
 
 The problem with the netlink approach is that auditing is not as good because 
 netlink is an async protocol. The kernel can only use the credentials that 
 ride in the skb with the command since there is no guarantee the process has 
 not changed credentials by the time you get the packet. Adding more 
 credentials to the netlink headers is also not good. As it is now, the 
 auditing is synchronous with the syscall and we can get at more information 
 and reliably create the audit records called out by the protection profiles 
 or 
 FIPS-140 level 2.
 
 -Steve

I think thats pretty easy to serialize though.  All you need to do is enforce a
rule in the kernel that dictates any creditial changes to a given context must
be serialized behind all previously submitted crypto operations.  It might make
life a bit tougher on the audit code, but netlink contains pid/sequence data in
all messages so that audit can correlate requests and responses easily.

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

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Tue, Aug 10, 2010 at 10:47:14AM -0400, Miloslav Trmac wrote:
 
 - Neil Horman nhor...@redhat.com wrote:
  On Tue, Aug 10, 2010 at 09:24:31AM -0400, Steve Grubb wrote:
Thats why I had suggested the use of a netlink protocol to manage this 
kind
of interface.  There are other issue to manage there, but they're really
not that big a deal, comparatively speaking, and that interface allows 
for
the easy specification of arbitrary length data in a fashion that 
doesn't
cause user space breakage when additional algorithms/apis are added.
   
   The problem with the netlink approach is that auditing is not as good 
   because 
   netlink is an async protocol. The kernel can only use the credentials 
   that 
   ride in the skb with the command since there is no guarantee the process 
   has 
   not changed credentials by the time you get the packet. Adding more 
   credentials to the netlink headers is also not good. As it is now, the 
   auditing is synchronous with the syscall and we can get at more 
   information 
   and reliably create the audit records called out by the protection
  profiles or 
   FIPS-140 level 2.
   
   -Steve
  
  I think thats pretty easy to serialize though.  All you need to do is 
  enforce a
  rule in the kernel that dictates any creditial changes to a given context 
  must
  be serialized behind all previously submitted crypto operations.
 That would be a bit unusual from the layering/semantics aspect - why should 
 credential changes care about crypto operations when they don't care about 
 any other operations? - and it would require pretty widespread changes 
 throughout the kernel core.
 Mirek


I'm sorry, I thought steve was referring to credentials in the sense of changing
keys/etc while crypto operations were in flight.  If you're referring to users
credentials in terms of permissions to use a device or service, then I think its
all moot anyway.  As you say why should credential changes care about crypto ops
when they don't care about other ops.  If you have permissions to use a
device/service, changing those permissions doesnt really change the fact that
you're in the process of using that service.  We could do some modicum of
credential check when requests are submitted and deny service based on that
check, but anything already submitted was done when you had permission to do so
and should be allowed to complete.

Neil

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

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Tue, Aug 10, 2010 at 11:36:16AM -0400, Miloslav Trmac wrote:
 
 - Neil Horman nhor...@tuxdriver.com wrote:
  On Mon, Aug 09, 2010 at 08:00:55PM -0400, Miloslav Trmac wrote:
   Is the proposed interface acceptable in the general approach (enums
  for algorithms/operations, unions for parameters, session
  init/update/finalize)?  With respect to flexibility, do you have
  specific suggestions or areas of concern?
  
  I know we spoke about this previously, but since you're asking publically, 
  I'll
  make my argument here so that we can have the debate in public as well.  
  I'm ok
  wih the general enumeration of operations in user space (I honestly can't 
  see
  how you would do it otherwise).  What worries me though is the use ioctl for
  these operations  and the flexibility that it denies you in future updates. 
  Specifically, my concerns are twofold:
  
  1) struct format.  By passing down a structure as your doing through an 
  ioctl
  call, theres  no way to extend/modify that structure easily for future use. 
   For
  instance the integration of aead might I think requires a blocking factor 
  to be
  sepcified, and entry for which you have no available field in your crypt_ops
  structure.  If you extend the structure in a later release of this code, you
  create a potential incompatibility with user space because you are no longer
  guaranteed that the size of the crypt_op structure is the same, and you 
  need to
  be prepared for a range of sizes to get passed down, at which point it 
  becomes
  difficult to differentiate between older code thats properly formatted, and
  newer code thats legitimately broken.  You might could add a version field 
  to
  mitigate that, but it gets ugly pretty quickly.
 
 I think it would be useful to separate thinking about the data format and 
 about the transmission mechanism.  ioctl() can quite well be used to carry 
 netlink-like packets - blobs of data with specified length and flexible 
 internal structure - and on the other hand, netlink could be (and often is) 
 used to carry fixed structs instead of variable-length packets.

Yes, both mechanism can be used to carry either fixed or variable length
payloads.  The difference is ioctl has no built in mechanism to do variable
length data safely.  To do variable length data you need to setup a bunch of
pointers to extra data that you have to copy separately.  Even then, you're
fixing the number of pointers that you have in your base structure.  To add or
remove any would break your communication protocol to user space.  This is
exactly the point I made above.

 
 So, any advantages netlink packets have in this respect can be provided using 
 the ioctl() interface as well.  Besides, adding new ioctl commands provides a 
 quite natural versioning mechanism (of course, it would be better to design 
 the data format right the first time, but the option to easily extend the 
 interface is available).  Versioned data structure that gets ugly pretty 
 quickly is exactly what one can get with the netlink packets, I think :) - 
 and it is not that bad actually.
No they can't.  I just explained again why it can't above.  At least not without
additional metadata and compatibility code built into the struct that you pass
in the ioctl.

Add commands is irrelevant.  Its equally easy to do so in an enumeration
provided in a struct as it is to do in a netlink message.  Theres no advantage
in either case there.

And there is no need for versioning in the netlink packet, because the data
types are all inlined, typed and attached to length values (at least when done
correctly, see then nl_attr structure and associated macros).  You don't have
that with your ioctl.  You could add it for certain, but at that point you're
re-inventing the wheel.

 
 Using a different design of the structures, perhaps appending a flexible 
 array of attributes at the end of each operation structure, is certainly 
 something to consider, if you think this is the way to go; the added 
 flexibility is undisputable, at the cost of some encoding/decoding overhead.

Thats exactly what netlink already has built in.  Each netlink message consistes
of a nlmsghdr structure which defines the source/dest process/etc.  Thats
followed by a variable number of nlattr attributes, each of which consists of a
type, length, and array of data bytes. We have kernel macros built to
encode/decode these attribtues already.  The work is already done (see the
nlmsg_parse function in the kernel).


 
 As for the transmission mechanism, netlink seems to me to be one of the least 
 desirable options: as described above, it does not provide any inherent 
 advantages to ioctl, and it has significantly higher overhead 
 (sendmsg()+recvmsg(), handling netlink ACKs, matching requests and replies in 
 multi-threaded programs), and it makes auditing difficult.

You're incorrect.  I've explained several of the advantiges above and in my 
previous
email, you're just not seeing

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Tue, Aug 10, 2010 at 11:40:00AM -0400, Miloslav Trmac wrote:
 - Neil Horman nhor...@redhat.com wrote:
  On Tue, Aug 10, 2010 at 10:47:14AM -0400, Miloslav Trmac wrote:
   - Neil Horman nhor...@redhat.com wrote:
On Tue, Aug 10, 2010 at 09:24:31AM -0400, Steve Grubb wrote:
 The problem with the netlink approach is that auditing is not as good 
 because 
 netlink is an async protocol. The kernel can only use the credentials 
 that 
 ride in the skb with the command since there is no guarantee the 
 process has 
 not changed credentials by the time you get the packet. Adding more 
 credentials to the netlink headers is also not good. As it is now, 
 the 
 auditing is synchronous with the syscall and we can get at more 
 information 
 and reliably create the audit records called out by the protection 
 profiles or 
 FIPS-140 level 2.
 
 -Steve

I think thats pretty easy to serialize though.  All you need to do is 
enforce a
rule in the kernel that dictates any creditial changes to a given 
context must
be serialized behind all previously submitted crypto operations.
   That would be a bit unusual from the layering/semantics aspect - why
  should credential changes care about crypto operations when they don't
  care about any other operations? - and it would require pretty
  widespread changes throughout the kernel core.
   Mirek
  
  
  I'm sorry, I thought steve was referring to credentials in the sense of 
  changing
  keys/etc while crypto operations were in flight.
 The audited values are mainly process/thread attributes: pid, ppid, 
 {,e,fs}[ug]id, session id, and the like.  Most of these are attached to the 
 netlink packet, and performing a lookup by PID is at packet handling time is 
 unreliable - as far as I understand the netlink receive routine does not have 
 to run in the same process context, so the PID might not even refer to the 
 same process.
 Mirek

I'm not so sure I follow.  how can you receive messages on a socket in response
to requests that were sent from a different socket.  In the netlink multicast
and broadcast case, sure, but theres no need to use those.  I suppose you could
exit a process, start another process in which the pid gets reused, open up a
subsequent socket and perhaps confuse audit that way, but you're not going to
get responses to the requests that the previous process sent in that case.  And
in the event that happens, Audit should log a close event on the fd inquestion
between the operations.


Theres also the child process case, in which a child process might read
responses from requests sent by a parent, and vice versa, since fork inherits
file descriptors, but thats an issue inherent in any mechanism you use,
including the character interface, so I'm not sure what the problem is there.

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

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Tue, Aug 10, 2010 at 12:57:43PM -0400, Miloslav Trmac wrote:
 
 - Neil Horman nhor...@redhat.com wrote:
 
  On Tue, Aug 10, 2010 at 11:40:00AM -0400, Miloslav Trmac wrote:
   - Neil Horman nhor...@redhat.com wrote:
On Tue, Aug 10, 2010 at 10:47:14AM -0400, Miloslav Trmac wrote:
 - Neil Horman nhor...@redhat.com wrote:
  On Tue, Aug 10, 2010 at 09:24:31AM -0400, Steve Grubb wrote:
   The problem with the netlink approach is that auditing is not as 
   good because
   netlink is an async protocol. The kernel can only use the 
   credentials that
   ride in the skb with the command since there is no guarantee the 
   process has
   not changed credentials by the time you get the packet. Adding 
   more
   credentials to the netlink headers is also not good. As it is 
   now, the
   auditing is synchronous with the syscall and we can get at more 
   information
   and reliably create the audit records called out by the 
   protection profiles or
   FIPS-140 level 2.
  
   -Steve
 
  I think thats pretty easy to serialize though.  All you need to do 
  is enforce a
  rule in the kernel that dictates any creditial changes to a given 
  context must
  be serialized behind all previously submitted crypto operations.
 That would be a bit unusual from the layering/semantics aspect - why
should credential changes care about crypto operations when they don't
care about any other operations? - and it would require pretty
widespread changes throughout the kernel core.
 Mirek
   
   
I'm sorry, I thought steve was referring to credentials in the sense of 
changing
keys/etc while crypto operations were in flight.
   The audited values are mainly process/thread attributes: pid, ppid,
  {,e,fs}[ug]id, session id, and the like.  Most of these are attached
  to the netlink packet, and performing a lookup by PID is at packet
  handling time is unreliable - as far as I understand the netlink
  receive routine does not have to run in the same process context, so
  the PID might not even refer to the same process.
 
  I'm not so sure I follow.  how can you receive messages on a socket in 
  response
  to requests that were sent from a different socket.  In the netlink 
  multicast
  and broadcast case, sure, but theres no need to use those.  I suppose you 
  could
  exit a process, start another process in which the pid gets reused, open up 
  a
  subsequent socket and perhaps confuse audit that way, but you're not going 
  to
  get responses to the requests that the previous process sent in that case.
 I don't even need to open a subsequent socket - as son as the process ID is 
 reused, the audit message is incorrect, which is not really acceptable in 
 itself.
 
But, you do, thats my point.  If a process exits, and another process starts up
that happens to reuse the same pid, it can't just call recvmsg on the socket
descriptor that the last process used for netlink messages and expect to get any
data.  That socket descriptor won't be connected to the netlink service (or
anything) anymore, and you'll get an error from the kernel.

  And
  in the event that happens, Audit should log a close event on the fd 
  inquestion
  between the operations.
 audit only logs explicitly requested operations, so an administrator that 
 asks for crypto events does not automatically get any close events.  Besides, 
 the audit record should be correct in the first place, instead of giving the 
 admin a puzzle to decipher.
I still don't see whats incorrect here.  If two processes wind up reusing a
process id, thats audits problem to figure out, nothing elses.  What exactly is
the problem that you see involving netlink and audit here?  Compare whatever
problem you see a crypto netlink protocol having in regards to audit to another
netlink protocol (say rtnetlink), and explain to me why the latter doesn't have
that issue as well.


 
  Theres also the child process case, in which a child process might read
  responses from requests sent by a parent, and vice versa, since fork 
  inherits
  file descriptors, but thats an issue inherent in any mechanism you use,
  including the character interface, so I'm not sure what the problem is
  there.
 Actually that's not a problem with ioctl() because the response is written 
 back _before_ ioctl() returns, so it is always provided to the original 
 calling thread.
 
 With the current design the parent and child share the key/session namespace 
 after fork(), but operation results are always reliably and automatically 
 provided to the thread that invoked the operation, without any user-space 
 collaboration.
Is that _really_ that important?  That the kernel return data in the same call
that its made?  I don't believe for a minute that FIPS has any madate on that.
I believe that it might be easier for audit to provide records on single calls,
rather than

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Tue, Aug 10, 2010 at 02:14:24PM -0400, Steve Grubb wrote:
 On Tuesday, August 10, 2010 01:57:40 pm Neil Horman wrote:
I'm not so sure I follow.  how can you receive messages on a socket in
response to requests that were sent from a different socket.  In the
netlink multicast and broadcast case, sure, but theres no need to use
those.  I suppose you could exit a process, start another process in
which the pid gets reused, open up a subsequent socket and perhaps
confuse audit that way, but you're not going to get responses to the
requests that the previous process sent in that case.
   
   I don't even need to open a subsequent socket - as son as the process ID
   is reused, the audit message is incorrect, which is not really
   acceptable in itself.
  
   
  
  But, you do, thats my point.  If a process exits, and another process
  starts up that happens to reuse the same pid, it can't just call recvmsg
  on the socket descriptor that the last process used for netlink messages
  and expect to get any data.  That socket descriptor won't be connected to
  the netlink service (or anything) anymore, and you'll get an error from
  the kernel.
 
 You are looking at it from the wrong end. Think of it from audit's 
 perspective 
 about how do you guarantee that the audit trail is correct? This has been 
 discussed on linux-audit mail list before and the conclusion is you have very 
 limited information to work with. By being synchronous the syscall, we get 
 everything in the syscall record from the processes audit context.
 

What information do you need in the audit record that you might loose accross
two syscalls?  It sounds from previous emails that, generally speaking, you're
worried that you want the task struct that current points to in the recvmsg call
be guaranteeed to be the same as the task struct that current points to in the
sendmsg call (i.e. no children (re)using the same socket descriptor, etc).  Can
this be handled by using the fact that netlink is actually syncronous under the
covers?  i.e. when you send a message to a netlink service, there is no reason
that all the relevant crypto ops in the request can't be completed in the
context of that call, as long as all your crypto operations are themselves
synchronous.  By the time you are done with the sendmsg call, you can know if
your entire crypto op is successfull.  The only thing that isn't complete is the
retrieval of the completed operations data from the kernel.  Is that enough to
make an audit log entry in the same way that an ioctl would?

 The audit logs require non-repudiation. It cannot be racy or stitch together 
 possibly wrong events. Audit logs can and do wind up in court and we do not 
 want problems with any part of the system.
 
And in the event that happens, Audit should log a close event on the fd
inquestion between the operations.
   
   audit only logs explicitly requested operations, so an administrator that
   asks for crypto events does not automatically get any close
   events.  Besides, the audit record should be correct in the first place,
   instead of giving the admin a puzzle to decipher.
  
  I still don't see whats incorrect here.  If two processes wind up reusing a
  process id, thats audits problem to figure out, nothing elses
 
 True, but that is the point of this exercise - meeting common criteria and 
 FIPS. They both have rules about what the audit logs should present and the 
 assuarnce that the information is correct and not racy.
 

Can you ennumerate here what FIPS and Common Criteria mandate be presented in
the audit logs? 

Neil

  .  What exactly is the problem that you see involving netlink and audit
  here?  Compare whatever problem you see a crypto netlink protocol having
  in regards to audit to another netlink protocol (say rtnetlink), and
  explain to me why the latter doesn't have that issue as well.
 
 That one is not security sensitive. Nowhere in any protection profile does it 
 say to audit that.
 
 -Steve
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Tue, Aug 10, 2010 at 02:19:59PM -0400, Miloslav Trmac wrote:
 - Neil Horman nhor...@tuxdriver.com wrote:
  On Tue, Aug 10, 2010 at 12:57:43PM -0400, Miloslav Trmac wrote:
   
   - Neil Horman nhor...@redhat.com wrote:
   
On Tue, Aug 10, 2010 at 11:40:00AM -0400, Miloslav Trmac wrote:
 - Neil Horman nhor...@redhat.com wrote:
  On Tue, Aug 10, 2010 at 10:47:14AM -0400, Miloslav Trmac wrote:
   - Neil Horman nhor...@redhat.com wrote:
On Tue, Aug 10, 2010 at 09:24:31AM -0400, Steve Grubb wrote:
 The problem with the netlink approach is that auditing is not 
 as good because
 netlink is an async protocol. The kernel can only use the 
 credentials that
 ride in the skb with the command since there is no guarantee 
 the process has
 not changed credentials by the time you get the packet. 
 Adding more
 credentials to the netlink headers is also not good. As it is 
 now, the
 auditing is synchronous with the syscall and we can get at 
 more information
 and reliably create the audit records called out by the 
 protection profiles or
 FIPS-140 level 2.

 -Steve
   
I think thats pretty easy to serialize though.  All you need to 
do is enforce a
rule in the kernel that dictates any creditial changes to a 
given context must
be serialized behind all previously submitted crypto operations.
   That would be a bit unusual from the layering/semantics aspect - 
   why
  should credential changes care about crypto operations when they 
  don't
  care about any other operations? - and it would require pretty
  widespread changes throughout the kernel core.
   Mirek
 
 
  I'm sorry, I thought steve was referring to credentials in the 
  sense of changing
  keys/etc while crypto operations were in flight.
 The audited values are mainly process/thread attributes: pid, ppid,
{,e,fs}[ug]id, session id, and the like.  Most of these are attached
to the netlink packet, and performing a lookup by PID is at packet
handling time is unreliable - as far as I understand the netlink
receive routine does not have to run in the same process context, so
the PID might not even refer to the same process.
   
I'm not so sure I follow.  how can you receive messages on a socket in 
response
to requests that were sent from a different socket.  In the netlink 
multicast
and broadcast case, sure, but theres no need to use those.  I suppose 
you could
exit a process, start another process in which the pid gets reused, 
open up a
subsequent socket and perhaps confuse audit that way, but you're not 
going to
get responses to the requests that the previous process sent in that 
case.
   I don't even need to open a subsequent socket - as son as the
  process ID is reused, the audit message is incorrect, which is not
  really acceptable in itself.
   
  But, you do, thats my point.  If a process exits, and another process 
  starts up
  that happens to reuse the same pid, it can't just call recvmsg on the socket
  descriptor that the last process used for netlink messages and expect to 
  get any
  data.
 It _doesn't matter_ that I don't receive a response.  I have caused an 
 operation in the kernel and the requested audit record is incorrect.  The 
 audit subsystem needs to work even if - especially if - the userspace process 
 is malicious.  The process might have wanted to simply cause a misleading 
 audit record; or the operation (such as importing a key from supplied data) 
 might not really need the response in order to achieve its effect.
 

Why is it incorrect?  What would audit have recorded in the above case?  That a
process attempted to recieve data on a descriptor that it didn't have open?
That seems correct to me.

And
in the event that happens, Audit should log a close event on the
  fd inquestion
between the operations.
   audit only logs explicitly requested operations, so an administrator
  that asks for crypto events does not automatically get any close
  events.  Besides, the audit record should be correct in the first
  place, instead of giving the admin a puzzle to decipher.
  I still don't see whats incorrect here.  If two processes wind up reusing a
  process id, thats audits problem to figure out, nothing elses.
 If an operation attributed to user A is recorded by the kernel as being 
 performed by user B, that is not an user problem.
 
You're right, its also not a netlink problem, a cryptodev problem, or an ioctl
problem.  Its an audit problem.



  What exactly is
  the problem that you see involving netlink and audit here?  Compare whatever
  problem you see a crypto netlink protocol having in regards to audit to 
  another
  netlink protocol (say rtnetlink), and explain

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Tue, Aug 10, 2010 at 03:10:12PM -0400, Steve Grubb wrote:
 On Tuesday, August 10, 2010 02:45:44 pm Neil Horman wrote:
  On Tue, Aug 10, 2010 at 02:14:24PM -0400, Steve Grubb wrote:
   On Tuesday, August 10, 2010 01:57:40 pm Neil Horman wrote:
  I'm not so sure I follow.  how can you receive messages on a socket
  in response to requests that were sent from a different socket. 
  In the netlink multicast and broadcast case, sure, but theres no
  need to use those.  I suppose you could exit a process, start
  another process in which the pid gets reused, open up a subsequent
  socket and perhaps confuse audit that way, but you're not going to
  get responses to the requests that the previous process sent in
  that case.
 
 I don't even need to open a subsequent socket - as son as the process
 ID is reused, the audit message is incorrect, which is not really
 acceptable in itself.

But, you do, thats my point.  If a process exits, and another process
starts up that happens to reuse the same pid, it can't just call
recvmsg on the socket descriptor that the last process used for
netlink messages and expect to get any data.  That socket descriptor
won't be connected to the netlink service (or anything) anymore, and
you'll get an error from the kernel.
   
   You are looking at it from the wrong end. Think of it from audit's
   perspective about how do you guarantee that the audit trail is correct?
   This has been discussed on linux-audit mail list before and the
   conclusion is you have very limited information to work with. By being
   synchronous the syscall, we get everything in the syscall record from
   the processes audit context.
  
  What information do you need in the audit record that you might loose
  accross two syscalls?
 
 This is easier to show that explain. With Mirek's current patch:
 
 type=SYSCALL msg=audit(1281013374.713:11671): arch=c03e syscall=2 
 success=yes exit=3 a0=400b67 a1=2 a2=0 a3=7fff4daa1200 items=1 ppid=1352 
 pid=1375 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 
 tty=tty6 ses=3 comm=ncr-setkey exe=/home/mitr/cryptodev-
 linux/userspace/ncr-setkey subj=unconfined_u:unconfined_r:unconfined_t:s0-
 s0:c0.c1023 key=(null)
 type=CRYPTO_USERSPACE_OP msg=audit(1281013374.713:11671): 
 crypto_op=context_new ctx=0
 type=CWD msg=audit(1281013374.713:11671):  cwd=/root
 type=PATH msg=audit(1281013374.713:11671): item=0 name=/dev/crypto 
 inode=12498 dev=00:05 mode=020660 ouid=0 ogid=0 rdev=0a:3a 
 obj=system_u:object_r:device_t:s0
 type=CRYPTO_STORAGE_KEY msg=audit(1281013374.715:11672): key_size=16
 
 
 The netlink aproach, we only get the credentials that ride with the netlink 
 packet 
 
 http://lxr.linux.no/#linux+v2.6.35/include/linux/netlink.h#L159
 
 There really is no comparison between what can be recorded synchronously vs 
 async.
 

Ok, so the $64 dollar question then: Do FIPS or Common Criteria require that you
log more than whats in the netlink packet?

 
  It sounds from previous emails that, generally speaking, you're worried that
  you want the task struct that current points to in the recvmsg call be
  guaranteeed to be the same as the task struct that current points to in the
  sendmsg call (i.e. no children (re)using the same socket descriptor, etc). 
 
 Not really, we are _hoping_ that the pid in the netlink credentials is the 
 same pid that sent the packet in the first place. From the time we reference 
 the pid out of the skb until we go hunting and locate the pid in the list of 
 tasks, it could have died and been replaced with another task with different 
 credentials. We can't take that risk.
 
 
  Can this be handled by using the fact that netlink is actually syncronous
  under the covers?
 
 But its not or all the credentials would not be riding in the skb.
 
Not true.  It not guaranteed to, as you can do anything you want when you
receive a netlink msg in the kernel.  But thats not to say that you can't
enforce synchronization, and in so doing obtain more information.

 
  Can you ennumerate here what FIPS and Common Criteria mandate be presented
  in the audit logs?
 
 Who did what to whom at what time and what was the outcome. In the case of 
 configuration changes we need the new and old values. However, we need extra 
 information to make the selective audit work right.
 
Somehow I doubt that FIPS mandates that audit messages include who did what to
whoom and what the result was :).  Seriously, what do FIPS and common criteria
require in an audit message?  I assume this is a partial list:
* sending process id
* requested operation
* session id
* user id
* group id
* operation result

I see lots of other items in the above log message, but I'm not sure what else
is required.  Can you elaborate?

Neil

 -Steve
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Tue, Aug 10, 2010 at 02:58:01PM -0400, Miloslav Trmac wrote:
 - Neil Horman nhor...@tuxdriver.com wrote:
  On Tue, Aug 10, 2010 at 11:36:16AM -0400, Miloslav Trmac wrote:
   I think it would be useful to separate thinking about the data
  format and about the transmission mechanism.  ioctl() can quite well
  be used to carry netlink-like packets - blobs of data with specified
  length and flexible internal structure - and on the other hand,
  netlink could be (and often is) used to carry fixed structs instead of
  variable-length packets.
  
  Yes, both mechanism can be used to carry either fixed or variable length
  payloads.  The difference is ioctl has no built in mechanism to do variable
  length data safely.  To do variable length data you need to setup a bunch of
  pointers to extra data that you have to copy separately.
 No, I can do exactly what netlink does:
 
 struct operation {
 // fixed params;
 size_t size_of_attrs;
 char attrs[]; // struct nlattr-tagged data
 };
 
 at the cost of one additional copy_from_user() (which netlink users often pay 
 as well because using struct iovec is so convenient).  Or perhaps even 
 better, I can make sure the userspace application won't mess up the pointer 
 arithmetic and get rid of all of the macros:
 
 struct operation {
 // fixed params;
 size_t num_attrs;
 struct { struct nlattr header; union { ... } data } attrs[];
 }
 
 at the same cost (and sacrificing string and variable-length parameters - but 
 as argued below, pointers are still an option).
 
  Even then, you're
  fixing the number of pointers that you have in your base structure.  To add 
  or
  remove any would break your communication protocol to user space.  This is
  exactly the point I made above.
 The existence of a base structure does not inherently limit the number of 
 extensions.
 
  And there is no need for versioning in the netlink packet, because the data
  types are all inlined, typed and attached to length values (at least when 
  done
  correctly, see then nl_attr structure and associated macros).  You don't 
  have
  that with your ioctl.  You could add it for certain, but at that point 
  you're
  re-inventing the wheel.
 Right, the nl_attr structure is quite nice, and I didn't know about it.  
 Still...
 
 
   As for the transmission mechanism, netlink seems to me to be one of
  the least desirable options: as described above, it does not provide
  any inherent advantages to ioctl, and it has significantly higher
  overhead (sendmsg()+recvmsg(), handling netlink ACKs, matching
  requests and replies in multi-threaded programs), and it makes
  auditing difficult.
  
  You're incorrect.  I've explained several of the advantiges above and in my 
  previous
  email, you're just not seeing them.  I will grant you some additional 
  overhead
  in the use of of two system calls rather than one per operation in the 
  nominal
  case, but there is no reason that can't be mitigated by the bundling of 
  multiple
  operations into a single netlink packet. 
 None of the existing crypto libraries provide such interfaces, and 
 restructuring applications to take advantage of them would often be difficult 
 - just restructuring the NSS _internals_ to support this would be difficult.  
 Porting applications to a Linux-specific interface would make them less 
 portable; besides, we have tried porting applications to a different library 
 before (http://fedoraproject.org/wiki/FedoraCryptoConsolidation ), and based 
 on that experience, any such new interface would have minimal, if any, impact.
 
  Likewise, matching requests and responses in a multi-threaded program is 
  also an
  already solved issue in multiple ways.  The use of multiple sockets, in a 1 
  per
  thread fashion is the most obvious.
 That would give each thread a separate namespace of keys/sessions, rather 
 strange and a problem to fit into existing applications.
 
  Theres also countless approaches in which a
  thread can reassemble responses to registered requests in such a way that 
  the
  user space portion of an application sees these calls as being synchronous. 
   Its
  really not that hard.
 The overhead is still unnecessary.
 
2) Use of pointers.  Thats just a pain.  You have the compat ioctl 
stuff in
place to handle the 32/64 bit conversions, but it would be really nice 
if you
could avoid having to maintain that extra code path.
   Pointers are pretty much unavoidable, to allow zero-copy references
  to input/output data.  If that means maintaining 32-bit compat paths
  (as opposed to codifying say uint128_t for pointers in fixed-size
  structures), then so be it: the 32-bit paths will simply have to be
  maintained.  Once we have the 32-bit compat paths, using pointers for
  other unformatted, variable-sized data (e.g. IVs, multiple-precision
  integers) seems to be easier than using variable-size data structures
  or explicit pointer arithmetic to compute data

Re: [PATCH 0/4] RFC: New /dev/crypto user-space interface

[Neil Horman]

On Thu, Aug 05, 2010 at 10:17:53PM +0200, Miloslav Trmač wrote:
 Hello,
 following is a patchset providing an user-space interface to the kernel crypto
 API.  It is based on the older, BSD-compatible, implementation, but the
 user-space interface is different.
 

I only see patch 1/4 and 3/4.  Where are 2/4 and 4/4?
Neil


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

Re: Fixing gave up waiting for init of module libcrc32c.

[Neil Horman]

On Sat, Mar 20, 2010 at 08:29:59PM +0800, Herbert Xu wrote:
 On Fri, Mar 19, 2010 at 10:23:25PM -0700, David Miller wrote:
 
  I hear what you're saying Herbert, but thinking about this a bit I
  really think we should make this situation work instead of fail.
 
 I think the initial report perhaps painted this in a slight
 different fashion than what it really is.  The code that was
 looping in module.c is not trying to load libcrc32c, but rather
 it is trying to get a reference on the already-loaded libcrc32c
 module.
 
 AFAICS the only way to make it work would be to reload the
 module in question when we can't get a reference on it.  But
 that would entail recursively loading a module during the process
 of loading another module.
 
 Rusty can chime in on whether this is doable.
 
 I think I have a good guess as to why this problem is occuring
 for Brandon.  It is probably the result of two near-simultaneous
 modprobes, one issued against libcrc32c and one against bnx2x.
 
 The libcrc32c module is partially loaded to the point of invoking
 its init function, which then tries to modprobe crc32c.
 
 However, before this starts the modprobe on bnx2x is already in
 progression.  When bnx2x's loading tries to acquire a reference
 on libcrc32c which it depends on, we hit the dead-lock.
 
 So if Suse were doing some kind of parallel booting where multiple
 modules may be loaded together then this could occur.
 
 The easiest solution again would be for modprobe(8) to block the
 loading of bnx2x because the module that it depends on libcrc32c
 hasn't yet finished loading.
 
 I'm open to a kernel solution too if anyone has suggestions.
 

FWIW, this sounds like a regression in modprobe to me.  A few years ago I fixed
a deadlock condition in the netfilter conntrack code that was tickled by
parallel rmmod's and modprobes.  modprobe would take file locks on modules, and
if the same module was getting rmmodded and modprobed in parallel we'd wind up
with a deadlock.  I fixed it by making the default modprobe -r behavior to be
non-blocking (which is the same as rmmod).  That commit is here:
http://git.kernel.org/?p=utils/kernel/module-init-tools/module-init-tools.git;a=commit;h=b45a24e9c89a14baf63bffe0a9ff04c1c1bffb29

Later, in late 2009, That behavior was reverted:
http://git.kernel.org/?p=utils/kernel/module-init-tools/module-init-tools.git;a=commit;h=b45a24e9c89a14baf63bffe0a9ff04c1c1bffb29

withuot consideration of the consequences, of which this sounds like one.

JCM I think is working on fixing the problem in a sane way.  I'd suggested that
he reapply the patch, but IIRC he told me that hes planning on trying to fix it
by removing the file locking on the modules in userspace entirely, which I think
is also reasonable.

As a test, you might try massaging my old patch above into the latest
module-init-tools to see if it makes the problem go away.  Note, the result of
this will be that either the modprobe or rmmod will fail and will need to be
retried, but its non-fatal, and a retry is usually successful, as it moves the
rmmod and modprobe further apart in time.

Regards
Neil

 Cheers,
 -- 
 Visit Openswan at http://www.openswan.org/
 Email: Herbert Xu ~{PmVHI~} herb...@gondor.apana.org.au
 Home Page: http://gondor.apana.org.au/~herbert/
 PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: move fips_cprng_get_random and fips_cprng_reset to CONFIG_CRYPTO_FIPS

[Neil Horman]

On Fri, Nov 20, 2009 at 07:20:19PM +0530, Jaswinder Singh Rajput wrote:
 
 fips_cprng_get_random and fips_cprng_reset is used only by
 CONFIG_CRYPTO_FIPS. This also fixes compilation warnings:
 
  crypto/ansi_cprng.c:360: warning: ‘fips_cprng_get_random’ defined but not 
 used
  crypto/ansi_cprng.c:393: warning: ‘fips_cprng_reset’ defined but not used
 
 Signed-off-by: Jaswinder Singh Rajput jaswinderraj...@gmail.com
Yeah, looks good to me, thanks!
Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH] crypto: Fix test in get_prng_bytes()

[Neil Horman]

On Mon, Oct 12, 2009 at 04:22:05PM +0200, Roel Kluin wrote:
 Op 12-10-09 16:07, Herbert Xu schreef:
  On Mon, Oct 12, 2009 at 09:51:42AM -0400, Neil Horman wrote:
  .
  Or should this test be removed?
 
  diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
  index 3aa6e38..9162456 100644
  --- a/crypto/ansi_cprng.c
  +++ b/crypto/ansi_cprng.c
  @@ -192,7 +192,7 @@ static int get_prng_bytes(char *buf, size_t nbytes, 
  struct prng_context *ctx)
int err;
   
   
  - if (nbytes  0)
  + if ((ssize_t)nbytes  0)
return -EINVAL;
   
spin_lock_bh(ctx-prng_lock);
  No, you're quite right, its a harmless, but unneeded check.  Herbert, 
  could you
  pull this into cryptodev please?  Thank you.
  
  Hmm, if it's unneeded why don't we just kill it instead?
 
 In that case:
 --8--8-
 size_t nbytes cannot be less than 0 and the test was redundant.
 
 Acked-by: Neil Horman nhor...@tuxdriver.com
 Signed-off-by: Roel Kluin roel.kl...@gmail.com
 ---
 diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
 index 3aa6e38..47995ae 100644
 --- a/crypto/ansi_cprng.c
 +++ b/crypto/ansi_cprng.c
 @@ -192,9 +192,6 @@ static int get_prng_bytes(char *buf, size_t nbytes, 
 struct prng_context *ctx)
   int err;
  
  
 - if (nbytes  0)
 - return -EINVAL;
 -
   spin_lock_bh(ctx-prng_lock);
  
   err = -EINVAL;
 


There you go, yes :)
Acked-by: Neil Horman nhor...@tuxdriver.com
Neil

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

Re: [PATCH 0/1] enhance RNG api with flags to allow for different operational modes (v2)

[Neil Horman]

 Hey all-
Ok, so I've got a story behind this one.  It was recently called to my
 attention that the ansi cprng is missing an aspect of its compliance 
requrements
 for FIPS-140.  Specifically, its missing a behavior in its continuous test.
 When the CPRNG produces random blocks, the firrst block that it produces must
 never be returned to the user.  Instead it must be saved and a second block 
must
 be generated so that it can be compared to the first block before being 
returned
 to the user.
 
I recently posted a patch to do this for the hardware RNG.  Its fine to
 do this there, since there are no expectations of a predictable result in that
 RNG.  The CPRNG however, provides a predictable random sequence for a given
 input seed key and iteration.  The above requirement messes with that
 predictability however because it changes which block is returned on the zeroth
 iteration to the user.  Some test vectors expect this, some do not.
 
So the question is, how do I make this RNG fips compliant without
 breaking some subset of users out there that rely on the predictability of the
 CPRNG? The solution we've discussed here is the use of a wrapper algorithm.  We
implement fips(ansi_cprng), which is exactly like the ansi_cprng, except that it
implements the continuous test on top of it.
 
 Signed-off-by: Neil Horman nhor...@tuxdriver.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] add fips(ansi_cprng) (v2)

[Neil Horman]

Patch to add fips(ansi_cprng) alg, which is ansi_cprng plus a continuous test

Signed-off-by: Neil Horman nhor...@tuxdriver.com

 ansi_cprng.c |   79 ---
 1 file changed, 70 insertions(+), 9 deletions(-)

diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 3aa6e38..027176a 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -85,7 +85,7 @@ static void xor_vectors(unsigned char *in1, unsigned char 
*in2,
  * Returns DEFAULT_BLK_SZ bytes of random data per call
  * returns 0 if generation succeded, 0 if something went wrong
  */
-static int _get_more_prng_bytes(struct prng_context *ctx)
+static int _get_more_prng_bytes(struct prng_context *ctx, int cont_test)
 {
int i;
unsigned char tmp[DEFAULT_BLK_SZ];
@@ -132,7 +132,7 @@ static int _get_more_prng_bytes(struct prng_context *ctx)
 */
if (!memcmp(ctx-rand_data, ctx-last_rand_data,
DEFAULT_BLK_SZ)) {
-   if (fips_enabled) {
+   if (cont_test) {
panic(cprng %p Failed repetition 
check!\n,
ctx);
}
@@ -185,7 +185,8 @@ static int _get_more_prng_bytes(struct prng_context *ctx)
 }
 
 /* Our exported functions */
-static int get_prng_bytes(char *buf, size_t nbytes, struct prng_context *ctx)
+static int get_prng_bytes(char *buf, size_t nbytes, struct prng_context *ctx,
+   int do_cont_test)
 {
unsigned char *ptr = buf;
unsigned int byte_count = (unsigned int)nbytes;
@@ -220,7 +221,7 @@ static int get_prng_bytes(char *buf, size_t nbytes, struct 
prng_context *ctx)
 
 remainder:
if (ctx-rand_data_valid == DEFAULT_BLK_SZ) {
-   if (_get_more_prng_bytes(ctx)  0) {
+   if (_get_more_prng_bytes(ctx, do_cont_test)  0) {
memset(buf, 0, nbytes);
err = -EINVAL;
goto done;
@@ -247,7 +248,7 @@ empty_rbuf:
 */
for (; byte_count = DEFAULT_BLK_SZ; byte_count -= DEFAULT_BLK_SZ) {
if (ctx-rand_data_valid == DEFAULT_BLK_SZ) {
-   if (_get_more_prng_bytes(ctx)  0) {
+   if (_get_more_prng_bytes(ctx, do_cont_test)  0) {
memset(buf, 0, nbytes);
err = -EINVAL;
goto done;
@@ -356,7 +357,15 @@ static int cprng_get_random(struct crypto_rng *tfm, u8 
*rdata,
 {
struct prng_context *prng = crypto_rng_ctx(tfm);
 
-   return get_prng_bytes(rdata, dlen, prng);
+   return get_prng_bytes(rdata, dlen, prng, 0);
+}
+
+static int fips_cprng_get_random(struct crypto_rng *tfm, u8 *rdata,
+   unsigned int dlen)
+{
+   struct prng_context *prng = crypto_rng_ctx(tfm);
+
+   return get_prng_bytes(rdata, dlen, prng, 1);
 }
 
 /*
@@ -384,6 +393,26 @@ static int cprng_reset(struct crypto_rng *tfm, u8 *seed, 
unsigned int slen)
return 0;
 }
 
+static int fips_cprng_reset(struct crypto_rng *tfm, u8 *seed, unsigned int 
slen)
+{
+   u8 rdata[DEFAULT_BLK_SZ];
+   int rc;
+
+   struct prng_context *prng = crypto_rng_ctx(tfm);
+
+   rc = cprng_reset(tfm, seed, slen);
+
+   if (!rc)
+   goto out;
+
+   /* this primes our continuity test */
+   rc = get_prng_bytes(rdata, DEFAULT_BLK_SZ, prng, 0);
+   prng-rand_data_valid = DEFAULT_BLK_SZ;
+
+out:
+   return rc;
+}
+
 static struct crypto_alg rng_alg = {
.cra_name   = stdrng,
.cra_driver_name= ansi_cprng,
@@ -404,19 +433,51 @@ static struct crypto_alg rng_alg = {
}
 };
 
+#ifdef CONFIG_CRYPTO_FIPS
+static struct crypto_alg fips_rng_alg = {
+   .cra_name   = fips(ansi_cprng),
+   .cra_driver_name= fips_ansi_cprng,
+   .cra_priority   = 300,
+   .cra_flags  = CRYPTO_ALG_TYPE_RNG,
+   .cra_ctxsize= sizeof(struct prng_context),
+   .cra_type   = crypto_rng_type,
+   .cra_module = THIS_MODULE,
+   .cra_list   = LIST_HEAD_INIT(rng_alg.cra_list),
+   .cra_init   = cprng_init,
+   .cra_exit   = cprng_exit,
+   .cra_u  = {
+   .rng = {
+   .rng_make_random= fips_cprng_get_random,
+   .rng_reset  = fips_cprng_reset,
+   .seedsize = DEFAULT_PRNG_KSZ + 2*DEFAULT_BLK_SZ,
+   }
+   }
+};
+#endif
 
 /* Module initalization */
 static int __init prng_mod_init(void)
 {
-   if (fips_enabled)
-   rng_alg.cra_priority += 200;
+   int rc = 0;
 
-   return crypto_register_alg(rng_alg

Re: [PATCH 0/3] enhance RNG api with flags to allow for different operational modes

[Neil Horman]

On Wed, Sep 16, 2009 at 10:37:29PM -0500, Herbert Xu wrote:
 On Wed, Sep 16, 2009 at 12:04:56PM -0400, Neil Horman wrote:
 
  So the question is, how do I make this RNG fips compliant without
  breaking some subset of users out there that rely on the predictability of 
  the
  CPRNG?  The solution I've come up with is a dynamic flag.  This patch series
 
 What user apart from the test vector relies on the predictability?
As Jarod mentioned, currently only the NIST certification vectors and, as a
result our testmgr vectors require disabling of the internal continuity test,
but to generalize from that, I would imagine that any set of certification
vectors that exist in the wild, may or may not assume the presence of the oth
iteration consumption, and this patch gives us the flexability to make use of
those.  I was thinking that this api extension could also be used for various
debugging purposes (additional flags could be created to enable internal
debugging, etc).

Neil

 -- 
 Visit Openswan at http://www.openswan.org/
 Email: Herbert Xu ~{PmVHI~} herb...@gondor.apana.org.au
 Home Page: http://gondor.apana.org.au/~herbert/
 PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
 --
 To unsubscribe from this list: send the line unsubscribe linux-crypto in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] enhance RNG api with flags to allow for different operational modes

[Neil Horman]

On Thu, Sep 17, 2009 at 08:39:51AM -0700, Herbert Xu wrote:
 On Thu, Sep 17, 2009 at 08:43:51AM -0400, Neil Horman wrote:
 
  As Jarod mentioned, currently only the NIST certification vectors and, as a
  result our testmgr vectors require disabling of the internal continuity 
  test,
  but to generalize from that, I would imagine that any set of certification
  vectors that exist in the wild, may or may not assume the presence of the 
  oth
  iteration consumption, and this patch gives us the flexability to make use 
  of
  those.  I was thinking that this api extension could also be used for 
  various
  debugging purposes (additional flags could be created to enable internal
  debugging, etc).
 
 My gut feeling would be to just get rid of the test vectors.
 
 But if you really want to keep them, please do it like CTR and
 RFC3686.  That is, have the raw RNG tested with the current vectors,
 and implement the FIPS version as a wrapper on top of it to remove
 the required bits.
 

Just so that I'm clear on what your suggesting, you're approach would be to
register two algs in ansi_cprng, a 'raw' cprng, and a 'fips compliant cprng'
underneath that used the raw cprng as a base, but implemented the continuity
test underneath it?  If so, yeah, I can get behind that idea.  I'll spin a new
set of patches shortly.

Neil

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

[PATCH 0/3] enhance RNG api with flags to allow for different operational modes

[Neil Horman]

Hey all-
Ok, so I've got a story behind this one.  It was recently called to my
attention that the ansi cprng is missing an aspect of its compliance requrements
for FIPS-140.  Specifically, its missing a behavior in its continuous test.
When the CPRNG produces random blocks, the firrst block that it produces must
never be returned to the user.  Instead it must be saved and a second block must
be generated so that it can be compared to the first block before being returned
to the user.

I recently posted a patch to do this for the hardware RNG.  Its fine to
do this there, since there are no expectations of a predictable result in that
RNG.  The CPRNG however, provides a predictable random sequence for a given
input seed key and iteration.  The above requirement messes with that
predictability however because it changes which block is returned on the zeroth
iteration to the user.  Some test vectors expect this, some do not.

So the question is, how do I make this RNG fips compliant without
breaking some subset of users out there that rely on the predictability of the
CPRNG?  The solution I've come up with is a dynamic flag.  This patch series
adds two api calls to the crypto RNG api rng_set_flags and rng_get_flags, which
set flags with global meaning on instances of an rng.  A given RNG can opt to
set the registered agorithm methods for these api calls or not.  In the event
they don't a default handler is set for each that returns EOPNOTSUPPORT.

Using this new mechanism I've implemented these calls in ansi_cprng so
that setting the TEST_MODE flag disables the continuous check, allowing for the
zeroth block to get returned to the user, which lets us pass most of the
supplied test vectors (most notably the NIST provided vectors).

Signed-off-by: Neil Horman nhor...@tuxdriver.com


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

Re: [PATCH 1/3] add RNG api calls to set common flags

[Neil Horman]

patch 1/3: Add flags infrastructure to rng api

This patch adds api calls for get/set flags calls to the crypto rng api. This
api allows algorithm implementations to register calls to respond to flag
settings that are global and common to all rng's.  If a given algorithm has no
external flags that it needs to respond to, it can opt to not register any
calls, and as a result a default handler will be registered for each which
universally returns EOPNOTSUPPORT.

Signed-off-by: Neil Horman nhor...@tuxdriver.com


 crypto/rng.c   |   13 +
 include/crypto/rng.h   |   21 +
 include/linux/crypto.h |9 +
 3 files changed, 43 insertions(+)

diff --git a/crypto/rng.c b/crypto/rng.c
index ba05e73..98f10b1 100644
--- a/crypto/rng.c
+++ b/crypto/rng.c
@@ -46,6 +46,17 @@ static int rngapi_reset(struct crypto_rng *tfm, u8 *seed, 
unsigned int slen)
return err;
 }
 
+static int rngapi_set_flags(struct crypto_rng *tfm, u8 flags)
+{
+   return -EOPNOTSUPP;
+}
+  
+static int rngapi_get_flags(struct crypto_rng *tfm, u8 *flags)
+{
+   return -EOPNOTSUPP;
+}
+  
+
 static int crypto_init_rng_ops(struct crypto_tfm *tfm, u32 type, u32 mask)
 {
struct rng_alg *alg = tfm-__crt_alg-cra_rng;
@@ -53,6 +64,8 @@ static int crypto_init_rng_ops(struct crypto_tfm *tfm, u32 
type, u32 mask)
 
ops-rng_gen_random = alg-rng_make_random;
ops-rng_reset = rngapi_reset;
+   ops-rng_set_flags = alg-rng_set_flags ? : rngapi_set_flags;
+   ops-rng_get_flags = alg-rng_get_flags ? : rngapi_get_flags;
 
return 0;
 }
diff --git a/include/crypto/rng.h b/include/crypto/rng.h
index c93f9b9..a639955 100644
--- a/include/crypto/rng.h
+++ b/include/crypto/rng.h
@@ -15,6 +15,17 @@
 
 #include linux/crypto.h
 
+/*
+ * RNG behavioral flags
+ * CRYPTO_RNG_TEST_MODE
+ *   places the RNG into a test mode for various certification tests.  Some
+ *   RNG's (most notably Deterministic RNGs) Can have internal tests which are 
+ *   required in normal operation mode, but affect the deterministic output 
+ *   of the RNG which throws some test vectors off, as they may not account for
+ *   these tests.  This flag allows us to disable the internal tests of an RNG.
+ */
+#define CRYPTO_RNG_TEST_MODE  0x01
+
 extern struct crypto_rng *crypto_default_rng;
 
 int crypto_get_default_rng(void);
@@ -72,4 +83,14 @@ static inline int crypto_rng_seedsize(struct crypto_rng *tfm)
return crypto_rng_alg(tfm)-seedsize;
 }
 
+static inline int crypto_rng_set_flags(struct crypto_rng *tfm, u8 flags)
+{
+   return crypto_rng_alg(tfm)-rng_set_flags(tfm, flags);
+}
+
+static inline int crypto_rng_get_flags(struct crypto_rng *tfm, u8 *flags)
+{
+   return crypto_rng_alg(tfm)-rng_get_flags(tfm, flags);
+}
+
 #endif
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index fd92988..6af8d2f 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -285,6 +285,10 @@ struct rng_alg {
   unsigned int dlen);
int (*rng_reset)(struct crypto_rng *tfm, u8 *seed, unsigned int slen);
 
+   int (*rng_set_flags)(struct crypto_rng *tfm, u8 flags);
+ 
+   int (*rng_get_flags)(struct crypto_rng *tfm, u8 *flags);
+
unsigned int seedsize;
 };
 
@@ -421,6 +425,11 @@ struct rng_tfm {
int (*rng_gen_random)(struct crypto_rng *tfm, u8 *rdata,
  unsigned int dlen);
int (*rng_reset)(struct crypto_rng *tfm, u8 *seed, unsigned int slen);
+
+   int (*rng_set_flags)(struct crypto_rng *tfm, u8 flags);
+ 
+   int (*rng_get_flags)(struct crypto_rng *tfm, u8 *flags);
+ 
 };
 
 #define crt_ablkcipher crt_u.ablkcipher
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] augment the testmgr code to set TEST_MODE flag on all rng instances

[Neil Horman]

patch 2/3: Update testmgr code to place any rng it tests in TEST_MODE

This patch instructs the testmgr code to place all rng allocations that it makes
into test mode, so that in the event that it has internal mechanisms that may
affect the testing of the RNG, they won't affect the outcome of the test they
are preforming.

Signed-off-by: Neil Horman nhor...@tuxdriver.com


 testmgr.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 6d5b746..89ea8c1 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1470,6 +1470,8 @@ static int alg_test_cprng(const struct alg_test_desc 
*desc, const char *driver,
return PTR_ERR(rng);
}
 
+   crypto_rng_set_flags(rng, CRYPTO_RNG_TEST_MODE);
+
err = test_cprng(rng, desc-suite.cprng.vecs, desc-suite.cprng.count);
 
crypto_free_rng(rng);
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] augment CPRNG to correctly implement continuous test for FIPS, and support TEST_MODE flags

[Neil Horman]

patch 3/3: modify cprng to make contnuity check fips compliant and allow for a
disabling of the continuity test when the RNG is placed in FIPS mode

Signed-off-by: Neil Horman nhor...@txudriver.com


 ansi_cprng.c |   56 +++-
 1 file changed, 51 insertions(+), 5 deletions(-)

diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 3aa6e38..c1ba5e8 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -33,6 +33,7 @@
 
 #define PRNG_FIXED_SIZE 0x1
 #define PRNG_NEED_RESET 0x2
+#define PRNG_DISABLE_CONT_TEST 0x3
 
 /*
  * Note: DT is our counter value
@@ -85,7 +86,7 @@ static void xor_vectors(unsigned char *in1, unsigned char 
*in2,
  * Returns DEFAULT_BLK_SZ bytes of random data per call
  * returns 0 if generation succeded, 0 if something went wrong
  */
-static int _get_more_prng_bytes(struct prng_context *ctx)
+static int _get_more_prng_bytes(struct prng_context *ctx, int test)
 {
int i;
unsigned char tmp[DEFAULT_BLK_SZ];
@@ -130,6 +131,9 @@ static int _get_more_prng_bytes(struct prng_context *ctx)
 * First check that we didn't produce the same
 * random data that we did last time around through this
 */
+   if (ctx-flags  PRNG_DISABLE_CONT_TEST)
+   goto skip_test;
+
if (!memcmp(ctx-rand_data, ctx-last_rand_data,
DEFAULT_BLK_SZ)) {
if (fips_enabled) {
@@ -146,7 +150,7 @@ static int _get_more_prng_bytes(struct prng_context *ctx)
}
memcpy(ctx-last_rand_data, ctx-rand_data,
DEFAULT_BLK_SZ);
-
+skip_test:
/*
 * Lastly xor the random data with I
 * and encrypt that to obtain a new secret vector V
@@ -220,7 +224,7 @@ static int get_prng_bytes(char *buf, size_t nbytes, struct 
prng_context *ctx)
 
 remainder:
if (ctx-rand_data_valid == DEFAULT_BLK_SZ) {
-   if (_get_more_prng_bytes(ctx)  0) {
+   if (_get_more_prng_bytes(ctx, 1)  0) {
memset(buf, 0, nbytes);
err = -EINVAL;
goto done;
@@ -247,7 +251,7 @@ empty_rbuf:
 */
for (; byte_count = DEFAULT_BLK_SZ; byte_count -= DEFAULT_BLK_SZ) {
if (ctx-rand_data_valid == DEFAULT_BLK_SZ) {
-   if (_get_more_prng_bytes(ctx)  0) {
+   if (_get_more_prng_bytes(ctx, 1)  0) {
memset(buf, 0, nbytes);
err = -EINVAL;
goto done;
@@ -306,7 +310,6 @@ static int reset_prng_context(struct prng_context *ctx,
memset(ctx-rand_data, 0, DEFAULT_BLK_SZ);
memset(ctx-last_rand_data, 0, DEFAULT_BLK_SZ);
 
-   ctx-rand_data_valid = DEFAULT_BLK_SZ;
 
ret = crypto_cipher_setkey(ctx-tfm, prng_key, klen);
if (ret) {
@@ -317,6 +320,18 @@ static int reset_prng_context(struct prng_context *ctx,
 
ret = 0;
ctx-flags = ~PRNG_NEED_RESET;
+
+   /*
+* If we don't disable the continuity test
+* we need to seed the n=0 iteration for test
+* comparison, so we get a first block here that
+* we never return to the user
+*/
+   ctx-rand_data_valid = DEFAULT_BLK_SZ;
+   if (!(ctx-flags  PRNG_DISABLE_CONT_TEST))
+   _get_more_prng_bytes(ctx, 0);
+   
+   ctx-rand_data_valid = DEFAULT_BLK_SZ;
 out:
spin_unlock_bh(ctx-prng_lock);
return ret;
@@ -384,6 +399,35 @@ static int cprng_reset(struct crypto_rng *tfm, u8 *seed, 
unsigned int slen)
return 0;
 }
 
+  /*
+   *  These are the registered functions which export behavioral flags
+   *  to the crypto api.  Most rng's don't have flags, but some might, like
+   *  the cprng which implements a 'test mode' for validation of vecotors
+   *  which disables the internal continuity tests
+   */
+static int cprng_set_flags(struct crypto_rng *tfm, u8 flags)
+{
+   struct prng_context *prng = crypto_rng_ctx(tfm);
+  
+   if (flags  CRYPTO_RNG_TEST_MODE)
+   prng-flags |= PRNG_DISABLE_CONT_TEST;
+  
+   return 0;
+}
+  
+  static int cprng_get_flags(struct crypto_rng *tfm, u8 *flags)
+{
+   struct prng_context *prng = crypto_rng_ctx(tfm);
+  
+   *flags = 0;
+  
+   if (prng-flags  PRNG_DISABLE_CONT_TEST)
+   *flags |= CRYPTO_RNG_TEST_MODE;
+  
+   return 0;
+}
+  
+
 static struct crypto_alg rng_alg = {
.cra_name   = stdrng,
.cra_driver_name= ansi_cprng,
@@ -399,6 +443,8 @@ static struct crypto_alg rng_alg = {
.rng = {
.rng_make_random= cprng_get_random,
.rng_reset

Re: [PATCH]: fix repetition test for hardware RNG to be FIPS compliant (v2)

[Neil Horman]

Ok, version 2 of the patch, taking comments into account

To be fips compliant, RNGs need to preform a continuous test on their output.
Specifically the requirement is that the first block of random data generated in
an RNG be saved to see the comparison test, and never returned to the caller.
This patch augments the continuous test in the hardware RNG to enforce this
requirement, making the hardware RNG fips compliant (when operating in fips
mode).

Neil

Signed-off-by: Neil Horman nhor...@tuxdriver.com



 random.c |   28 
 1 file changed, 20 insertions(+), 8 deletions(-)


diff --git a/drivers/char/random.c b/drivers/char/random.c
index d8a9255..36fb05e 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -399,6 +399,14 @@ module_param(debug, bool, 0644);
  * storing entropy in an entropy pool.
  *
  **/
+#define EXTRACT_SIZE 10
+
+#define REP_CHECK_BLOCK_COPIED 1
+
+struct repetition_check {
+   __u8 last_data[EXTRACT_SIZE];
+   __u8 flags;
+};
 
 struct entropy_store;
 struct entropy_store {
@@ -414,7 +422,7 @@ struct entropy_store {
unsigned add_ptr;
int entropy_count;
int input_rotate;
-   __u8 *last_data;
+   struct repetition_check rep;
 };
 
 static __u32 input_pool_data[INPUT_POOL_WORDS];
@@ -714,7 +722,6 @@ void add_disk_randomness(struct gendisk *disk)
 }
 #endif
 
-#define EXTRACT_SIZE 10
 
 /*
  *
@@ -855,19 +862,27 @@ static ssize_t extract_entropy(struct entropy_store *r, 
void *buf,
ssize_t ret = 0, i;
__u8 tmp[EXTRACT_SIZE];
unsigned long flags;
+   size_t saved_nbytes = nbytes;
 
+repeat_extract:
xfer_secondary_pool(r, nbytes);
nbytes = account(r, nbytes, min, reserved);
 
while (nbytes) {
extract_buf(r, tmp);
 
-   if (r-last_data) {
+   if (fips_enabled) {
spin_lock_irqsave(r-lock, flags);
-   if (!memcmp(tmp, r-last_data, EXTRACT_SIZE))
+   if ((r-rep.flags  REP_CHECK_BLOCK_COPIED) 
+   !memcmp(tmp, r-rep.last_data, EXTRACT_SIZE))
panic(Hardware RNG duplicated output!\n);
-   memcpy(r-last_data, tmp, EXTRACT_SIZE);
+   memcpy(r-rep.last_data, tmp, EXTRACT_SIZE);
spin_unlock_irqrestore(r-lock, flags);
+   if (!(r-rep.flags  REP_CHECK_BLOCK_COPIED)) {
+   r-rep.flags |= REP_CHECK_BLOCK_COPIED;
+   nbytes = saved_nbytes;
+   goto repeat_extract;
+   }
}
i = min_t(int, nbytes, EXTRACT_SIZE);
memcpy(buf, tmp, i);
@@ -951,9 +966,6 @@ static void init_std_data(struct entropy_store *r)
now = ktime_get_real();
mix_pool_bytes(r, now, sizeof(now));
mix_pool_bytes(r, utsname(), sizeof(*(utsname(;
-   /* Enable continuous test in fips mode */
-   if (fips_enabled)
-   r-last_data = kmalloc(EXTRACT_SIZE, GFP_KERNEL);
 }
 
 static int rand_initialize(void)
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH]: fix repetition test for hardware RNG to be FIPS compliant

[Neil Horman]

Hey all-
A while back I implemented a repetition check in the hardware RNG to
make it FIPS compliant.  It was just pointed out to me that there was an item in
the requirement that I missed.  Namely, when operating in FIPS mode, the RNG
should save the first n bit block that it produces for use in the repetition
check, but not return it to the caller (opting instead to return the next n bit
block which passes the repetiiton check instead.  This patch corrects that.

Neil

Signed-off-by: Neil Horman nhor...@tuxdriver.com


 random.c |   27 ---
 1 file changed, 20 insertions(+), 7 deletions(-)


diff --git a/drivers/char/random.c b/drivers/char/random.c
index d8a9255..6700248 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -399,6 +399,12 @@ module_param(debug, bool, 0644);
  * storing entropy in an entropy pool.
  *
  **/
+#define EXTRACT_SIZE 10
+#define REP_CHECK_BLOCK_COPIED 1
+struct repetition_check {
+   __u8 last_data[EXTRACT_SIZE];
+   __u8 flags;
+};
 
 struct entropy_store;
 struct entropy_store {
@@ -414,7 +420,7 @@ struct entropy_store {
unsigned add_ptr;
int entropy_count;
int input_rotate;
-   __u8 *last_data;
+   struct repetition_check *rep;
 };
 
 static __u32 input_pool_data[INPUT_POOL_WORDS];
@@ -714,7 +720,6 @@ void add_disk_randomness(struct gendisk *disk)
 }
 #endif
 
-#define EXTRACT_SIZE 10
 
 /*
  *
@@ -856,18 +861,24 @@ static ssize_t extract_entropy(struct entropy_store *r, 
void *buf,
__u8 tmp[EXTRACT_SIZE];
unsigned long flags;
 
+repeat_extract:
xfer_secondary_pool(r, nbytes);
nbytes = account(r, nbytes, min, reserved);
 
while (nbytes) {
extract_buf(r, tmp);
 
-   if (r-last_data) {
+   if (r-rep) {
spin_lock_irqsave(r-lock, flags);
-   if (!memcmp(tmp, r-last_data, EXTRACT_SIZE))
+   if ((r-rep-flags  REP_CHECK_BLOCK_COPIED) 
+   !memcmp(tmp, r-rep-last_data, EXTRACT_SIZE))
panic(Hardware RNG duplicated output!\n);
-   memcpy(r-last_data, tmp, EXTRACT_SIZE);
+   memcpy(r-rep-last_data, tmp, EXTRACT_SIZE);
spin_unlock_irqrestore(r-lock, flags);
+   if (!(r-rep-flags  REP_CHECK_BLOCK_COPIED)) {
+   r-rep-flags |= REP_CHECK_BLOCK_COPIED;
+   goto repeat_extract;
+   }
}
i = min_t(int, nbytes, EXTRACT_SIZE);
memcpy(buf, tmp, i);
@@ -952,8 +963,10 @@ static void init_std_data(struct entropy_store *r)
mix_pool_bytes(r, now, sizeof(now));
mix_pool_bytes(r, utsname(), sizeof(*(utsname(;
/* Enable continuous test in fips mode */
-   if (fips_enabled)
-   r-last_data = kmalloc(EXTRACT_SIZE, GFP_KERNEL);
+   if (fips_enabled) {
+   r-rep = kmalloc(sizeof(struct repetition_check), GFP_KERNEL);
+   r-rep-flags = 0;
+   }
 }
 
 static int rand_initialize(void)
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: ansi_cprng - Fix module initialization

[Neil Horman]

On Tue, Aug 25, 2009 at 10:14:16AM +0200, Steffen Klassert wrote:
 Return the value we got from crypto_register_alg() instead of
 returning 0 in any case.
 
 Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
 ---
  crypto/ansi_cprng.c |9 +
  1 files changed, 1 insertions(+), 8 deletions(-)
 
 diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
 index 5357ba7..3aa6e38 100644
 --- a/crypto/ansi_cprng.c
 +++ b/crypto/ansi_cprng.c
 @@ -408,17 +408,10 @@ static struct crypto_alg rng_alg = {
  /* Module initalization */
  static int __init prng_mod_init(void)
  {
 - int ret = 0;
 -
   if (fips_enabled)
   rng_alg.cra_priority += 200;
  
 - ret = crypto_register_alg(rng_alg);
 -
 - if (ret)
 - goto out;
 -out:
 - return 0;
 + return crypto_register_alg(rng_alg);
  }
  
  static void __exit prng_mod_fini(void)
 -- 
 1.5.4.2
 
 
Thanks!

Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: crypto: ansi_cprng - Do not select FIPS

[Neil Horman]

On Fri, Aug 14, 2009 at 09:13:55PM +1000, Herbert Xu wrote:
 On Fri, Aug 14, 2009 at 06:58:29AM -0400, Neil Horman wrote:
 
  Yeah, I suppose, it just seemed like a hack to me, since it really is a 
  logical
  boolean, and we use it as such.  Ok, I'll look at fixing this soon.  Thanks!
 
 Well it wouldn't be the first time we had to hack around Kconfig
 logic, just look at those *2 symbols in crypto/Kconfig :)
 -- 
 Visit Openswan at http://www.openswan.org/
 Email: Herbert Xu ~{PmVHI~} herb...@gondor.apana.org.au
 Home Page: http://gondor.apana.org.au/~herbert/
 PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt


What about something like this?  It defaults the CPRNG to m and makes FIPS
dependent on the CPRNG.  That way you get a module build by default, but you can
change it to y manually during config and still satisfy the dependency, and if
you select N it disables FIPS as well.  I rather like that better than making
FIPS a tristate.  I just tested it out here and it seems to work well.  Let me
know what you think

Neil

Signed-off-by: Neil Horman nhor...@tuxdriver.com


 Kconfig |8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 1db0995..7623442 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -23,11 +23,13 @@ comment Crypto core or helper
 
 config CRYPTO_FIPS
bool FIPS 200 compliance
+   depends on CRYPTO_ANSI_CPRNG
help
  This options enables the fips boot option which is
  required if you want to system to operate in a FIPS 200
  certification.  You should say no unless you know what
- this is.
+ this is. Note that CRYPTO_ANSI_CPRNG is requred if this
+ option is selected
 
 config CRYPTO_ALGAPI
tristate
@@ -787,12 +789,14 @@ comment Random Number Generation
 
 config CRYPTO_ANSI_CPRNG
tristate Pseudo Random Number Generation for Cryptographic modules
+   default m
select CRYPTO_AES
select CRYPTO_RNG
help
  This option enables the generic pseudo random number generator
  for cryptographic modules.  Uses the Algorithm specified in
- ANSI X9.31 A.2.4
+ ANSI X9.31 A.2.4. Not this option must be enabled if CRYPTO_FIPS 
+ is selected
 
 source drivers/crypto/Kconfig
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: crypto: ansi_cprng - Do not select FIPS

[Neil Horman]

On Thu, Aug 13, 2009 at 09:29:55PM +1000, Herbert Xu wrote:
 On Fri, Jun 19, 2009 at 08:55:00AM -0400, Neil Horman wrote:
 
  Thanks!  Thats definately an oversight.  Likely I included it because I was
  implementing it as part of the FIPS effort.  The CPRNG definately works 
  fine,
  even if fips is disabled.  Although I think the relationship should be 
  reversed,
  not just removed, as FIPS support requires the use of the CPRNG.  Something 
  like
  this:
  
   
  commit d9645d88d97e81c6528f311ee126df79a0d27501
  Author: Neil Horman nhor...@tuxdriver.com
  Date:   Fri Jun 19 08:52:37 2009 -0400
  
  Fix CPRNG/FIPS dependency
  
  The ANSI CPRNG has no dependence on FIPS support.  FIPS support however,
  requires the use of the CPRNG.  Adjust that depedency relationship in 
  Kconfig
  
  Signed-off-by: Neil Horman nhor...@tuxdriver.com
 
 Hmm, I just noticed that all my crypto modules have been marked
 as built-in again because of this patch.
 
 As you're selecting a tristate from a bool, it causes CPRNG
 and everything under it to be built-in.
 
 I'm going to revert this patch.
 
Is there a good way to select a tristate from a bool?  The logic is the right
thing to do above, it just seems the mechanism comes up a bit short
Neil

 Cheers,
 -- 
 Visit Openswan at http://www.openswan.org/
 Email: Herbert Xu ~{PmVHI~} herb...@gondor.apana.org.au
 Home Page: http://gondor.apana.org.au/~herbert/
 PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] typo in crypto/rng.c

[Neil Horman]

On Fri, Aug 07, 2009 at 11:49:20PM -0700, Christian Kujau wrote:
 
 Correct a typo in crypto/rng.c
 
 Signed-off-by: Christian Kujau li...@nerdbynature.de
 
 --- linux-2.6-git/crypto/rng.c.orig   2009-08-08 08:43:49.227693375 +0200
 +++ linux-2.6-git/crypto/rng.c2009-08-08 08:44:54.735426448 +0200
 @@ -123,4 +123,4 @@ void crypto_put_default_rng(void)
  EXPORT_SYMBOL_GPL(crypto_put_default_rng);
  
  MODULE_LICENSE(GPL);
 -MODULE_DESCRIPTION(Random Number Genertor);
 +MODULE_DESCRIPTION(Random Number Generator);
 
 -- 
 BOFH excuse #229:
 
 wrong polarity of neutron flow
 

Thanks!

Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH] crypto: Update stdrng test name so that the testmgr finds it properly

[Neil Horman]

On Thu, Jul 02, 2009 at 04:32:59PM +0800, Herbert Xu wrote:
 On Thu, Jul 02, 2009 at 08:02:55AM +0200, Sebastian Andrzej Siewior wrote:
 
  @@ -1480,7 +1480,7 @@ static int alg_test_cprng(const struct alg_test_desc 
  *desc, const char *driver,
   /* Please keep this list sorted by algorithm name. */
   static const struct alg_test_desc alg_test_descs[] = {
 {
  -  .alg = ansi_cprng,
  +  .alg = stdrng,
 .test = alg_test_cprng,
 .fips_allowed = 1,
 .suite = {
  
  Don't you use this vector for _all_ stdrngs now? krng for instance will
  fail.
 
 Good cath!
 
Indeed, I didn't even think of that.

 What this really should do is test a specific implementation of
 stdrng.  Like this,
 
Yeah, that looks right
Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH v2] crypto/ansi prng: alloc cipher just in in init()

[Neil Horman]

On Wed, Jul 01, 2009 at 09:25:17AM +0200, Sebastian Andrzej Siewior wrote:
 * Neil Horman | 2009-06-30 20:06:48 [-0400]:
 
 I think this looks better, yeah, have you tested this?  If not, give it a 
 quick
 run please, and I'll ack it.
 I've built it and started
 | modprobe tcrypt mode=150
 and I ended up with:
 | alg: No test for stdrng (ansi_cprng)
 
 So this should be fine I guess.
 
 Neil
 
 Sebastian
 

Yeah, ok, that should be good, although I'll make a note and fix the aliasing
issue in the testmgr code

Acked-by: Neil Horman nhor...@tuxdriver.com

Herbert, can you pull this into cryptodev please?

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

[PATCH] crypto: Update stdrng test name so that the testmgr finds it properly

[Neil Horman]

Fix crypto testmgr reference to rng self tests.

It seems the ansi_cprng self tests get skipped currently.  It appears this is
because the test name is ansi_cprng, but the test is looked up by the cra_name
value of the algorithm in question, and ansi_cprng registers its cra_name as
stdrng.  This patch brings the two into line.

Signed-off-by: Neil Horman nhor...@tuxdriver.com


 testmgr.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index f9bea9d..3315a38 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1480,7 +1480,7 @@ static int alg_test_cprng(const struct alg_test_desc 
*desc, const char *driver,
 /* Please keep this list sorted by algorithm name. */
 static const struct alg_test_desc alg_test_descs[] = {
{
-   .alg = ansi_cprng,
+   .alg = stdrng,
.test = alg_test_cprng,
.fips_allowed = 1,
.suite = {
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto/ansi prng: use just a BH lock

[Neil Horman]

On Mon, Jun 29, 2009 at 11:44:30PM +0200, Sebastian Andrzej Siewior wrote:
 From: Sebastian Andrzej Siewior sebast...@breakpoint.cc
 
 the current code uses a mix of sping_lock()  spin_lock_irqsave(). This can
 lead to deadlock with the correct timming  cprng_get_random() + cprng_reset()
 sequence.
 I've converted them to bottom half locks since all three user grab just a BH
 lock so this runs probably in softirq :)
 
 Signed-off-by: Sebastian Andrzej Siewior sebast...@breakpoint.cc

There are more than just 3 users of this rng.  This is the rng that gets
allocated in the event that someone calls crypto_alloc_rng(stdrng), and
several ciphers use that, and in turn various ipsec implementations use those
ciphers.  That said, I've built it and tested it, and don't currently see any
users of this api from within interrupt context (i.e. no lockdep warnings).  I
think we may want to make this api accessible within interrupt context, but that
can probably wait until we have a user in said context, to find the best way to
do that.  Herbert, can you apply this to your tree?  Thanks!

Acked-by: Neil Horman nhor...@tuxdriver.com

 ---
  crypto/ansi_cprng.c |9 -
  1 files changed, 4 insertions(+), 5 deletions(-)
 
 diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
 index d80ed4c..ff00b58 100644
 --- a/crypto/ansi_cprng.c
 +++ b/crypto/ansi_cprng.c
 @@ -187,7 +187,6 @@ static int _get_more_prng_bytes(struct prng_context *ctx)
  /* Our exported functions */
  static int get_prng_bytes(char *buf, size_t nbytes, struct prng_context *ctx)
  {
 - unsigned long flags;
   unsigned char *ptr = buf;
   unsigned int byte_count = (unsigned int)nbytes;
   int err;
 @@ -196,7 +195,7 @@ static int get_prng_bytes(char *buf, size_t nbytes, 
 struct prng_context *ctx)
   if (nbytes  0)
   return -EINVAL;
  
 - spin_lock_irqsave(ctx-prng_lock, flags);
 + spin_lock_bh(ctx-prng_lock);
  
   err = -EINVAL;
   if (ctx-flags  PRNG_NEED_RESET)
 @@ -268,7 +267,7 @@ empty_rbuf:
   goto remainder;
  
  done:
 - spin_unlock_irqrestore(ctx-prng_lock, flags);
 + spin_unlock_bh(ctx-prng_lock);
   dbgprint(KERN_CRIT returning %d from get_prng_bytes in context %p\n,
   err, ctx);
   return err;
 @@ -287,7 +286,7 @@ static int reset_prng_context(struct prng_context *ctx,
   int rc = -EINVAL;
   unsigned char *prng_key;
  
 - spin_lock(ctx-prng_lock);
 + spin_lock_bh(ctx-prng_lock);
   ctx-flags |= PRNG_NEED_RESET;
  
   prng_key = (key != NULL) ? key : (unsigned char *)DEFAULT_PRNG_KEY;
 @@ -332,7 +331,7 @@ static int reset_prng_context(struct prng_context *ctx,
   rc = 0;
   ctx-flags = ~PRNG_NEED_RESET;
  out:
 - spin_unlock(ctx-prng_lock);
 + spin_unlock_bh(ctx-prng_lock);
  
   return rc;
  
 -- 
 1.6.3.3
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: add optional continuous repetition test to entropy store based rngs

[Neil Horman]

FIPS-140 requires that all random number generators implement continuous self
tests in which each extracted block of data is compared against the last block
for repetition.  The ansi_cprng implements such a test, but it would be nice if
the hw rng's did the same thing.  Obviously its not something thats always
needed, but it seems like it would be a nice feature to have on occasion.  I've
written the below patch which allows individual entropy stores to be flagged as
desiring a continuous test to be run on them as is extracted.  By default this
option is off, but is enabled in the event that fips mode is selected during
bootup.

Neil

Signed-off-by: Neil Horman nhor...@tuxdriver.com

diff --git a/crypto/internal.h b/crypto/internal.h
index fc76e1f..150d389 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -26,12 +26,6 @@
 #include linux/rwsem.h
 #include linux/slab.h
 
-#ifdef CONFIG_CRYPTO_FIPS
-extern int fips_enabled;
-#else
-#define fips_enabled 0
-#endif
-
 /* Crypto notification events. */
 enum {
CRYPTO_MSG_ALG_REQUEST,
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 8c74448..fbdfc70 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -250,6 +250,8 @@
 #include asm/irq.h
 #include asm/io.h
 
+#include crypto/algapi.h
+
 /*
  * Configuration information
  */
@@ -400,6 +402,7 @@ module_param(debug, bool, 0644);
  **/
 
 struct entropy_store;
+#define ENT_F_CONT_TEST 1
 struct entropy_store {
/* read-only data: */
struct poolinfo *poolinfo;
@@ -413,6 +416,8 @@ struct entropy_store {
unsigned add_ptr;
int entropy_count;
int input_rotate;
+   int flags;
+   __u8 *last_data;
 };
 
 static __u32 input_pool_data[INPUT_POOL_WORDS];
@@ -424,7 +429,9 @@ static struct entropy_store input_pool = {
.name = input,
.limit = 1,
.lock = __SPIN_LOCK_UNLOCKED(input_pool.lock),
-   .pool = input_pool_data
+   .pool = input_pool_data,
+   .flags = 0,
+   .last_data = NULL
 };
 
 static struct entropy_store blocking_pool = {
@@ -433,7 +440,9 @@ static struct entropy_store blocking_pool = {
.limit = 1,
.pull = input_pool,
.lock = __SPIN_LOCK_UNLOCKED(blocking_pool.lock),
-   .pool = blocking_pool_data
+   .pool = blocking_pool_data,
+   .flags = 0,
+   .last_data = NULL
 };
 
 static struct entropy_store nonblocking_pool = {
@@ -441,7 +450,9 @@ static struct entropy_store nonblocking_pool = {
.name = nonblocking,
.pull = input_pool,
.lock = __SPIN_LOCK_UNLOCKED(nonblocking_pool.lock),
-   .pool = nonblocking_pool_data
+   .pool = nonblocking_pool_data,
+   .flags = 0,
+   .last_data = NULL
 };
 
 /*
@@ -852,12 +863,21 @@ static ssize_t extract_entropy(struct entropy_store *r, 
void *buf,
 {
ssize_t ret = 0, i;
__u8 tmp[EXTRACT_SIZE];
+   unsigned long flags;
 
xfer_secondary_pool(r, nbytes);
nbytes = account(r, nbytes, min, reserved);
 
while (nbytes) {
extract_buf(r, tmp);
+
+   if (r-flags  ENT_F_CONT_TEST) {
+   spin_lock_irqsave(r-lock, flags);
+   if (!memcmp(tmp, r-last_data, EXTRACT_SIZE))
+   panic(Hardware RNG duplicated output!\n);
+   memcpy(r-last_data, tmp, EXTRACT_SIZE);
+   spin_unlock_irqrestore(r-lock, flags);
+   }
i = min_t(int, nbytes, EXTRACT_SIZE);
memcpy(buf, tmp, i);
nbytes -= i;
@@ -940,6 +960,14 @@ static void init_std_data(struct entropy_store *r)
now = ktime_get_real();
mix_pool_bytes(r, now, sizeof(now));
mix_pool_bytes(r, utsname(), sizeof(*(utsname(;
+   /* Enable continuous test in fips mode */
+   if (fips_enabled) {
+   r-last_data = kmalloc(EXTRACT_SIZE, GFP_KERNEL);
+   if (r-last_data)
+   r-flags |= ENT_F_CONT_TEST;
+   else
+   panic(Could not alloc data for rng test\n);
+   } 
 }
 
 static int rand_initialize(void)
diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
index 0105454..88e9535 100644
--- a/include/crypto/algapi.h
+++ b/include/crypto/algapi.h
@@ -20,6 +20,12 @@ struct module;
 struct rtattr;
 struct seq_file;
 
+#ifdef CONFIG_CRYPTO_FIPS
+extern int fips_enabled;
+#else
+#define fips_enabled 0
+#endif
+
 struct crypto_type {
unsigned int (*ctxsize)(struct crypto_alg *alg, u32 type, u32 mask);
unsigned int (*extsize)(struct crypto_alg *alg,
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: add optional continuous repetition test to entropy store based rngs

[Neil Horman]

On Thu, Jun 04, 2009 at 03:14:10PM -0500, Matt Mackall wrote:
 On Thu, 2009-06-04 at 15:50 -0400, Neil Horman wrote:
  FIPS-140 requires that all random number generators implement continuous 
  self
  tests in which each extracted block of data is compared against the last 
  block
  for repetition.  The ansi_cprng implements such a test, but it would be 
  nice if
  the hw rng's did the same thing.  Obviously its not something thats always
  needed, but it seems like it would be a nice feature to have on occasion.  
  I've
  written the below patch which allows individual entropy stores to be 
  flagged as
  desiring a continuous test to be run on them as is extracted.  By default 
  this
  option is off, but is enabled in the event that fips mode is selected during
  bootup.
  
  Neil
  
  Signed-off-by: Neil Horman nhor...@tuxdriver.com
  
  diff --git a/crypto/internal.h b/crypto/internal.h
  index fc76e1f..150d389 100644
  --- a/crypto/internal.h
  +++ b/crypto/internal.h
  @@ -26,12 +26,6 @@
   #include linux/rwsem.h
   #include linux/slab.h
   
  -#ifdef CONFIG_CRYPTO_FIPS
  -extern int fips_enabled;
  -#else
  -#define fips_enabled 0
  -#endif
  -
   /* Crypto notification events. */
   enum {
  CRYPTO_MSG_ALG_REQUEST,
  diff --git a/drivers/char/random.c b/drivers/char/random.c
  index 8c74448..fbdfc70 100644
  --- a/drivers/char/random.c
  +++ b/drivers/char/random.c
  @@ -250,6 +250,8 @@
   #include asm/irq.h
   #include asm/io.h
   
  +#include crypto/algapi.h
  +
 
 I think we'd rather not make random.c incestuous with crypto/.
 
Not sure what to do about this.  The intent is to provide the external reference
to the fips_enabled flag (which is either defined as an extern in or #defined to
0 dependent on CONFIG_CRYPTO_FIPS).  I can cut'n'paste the code block from the
include file and put it in here, but that seems like a worse solution to me.
Let me know your thoughts, and I can change this accordingly.

As for the other comments, they all seem good to me, let me know what you want
to do about the above, and I'll respin/repost the patch for you

Thanks!
Neil

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

Re: [PATCH] crypto: add optional continuous repetition test to entropy store based rngs

[Neil Horman]

On Fri, Jun 05, 2009 at 10:30:06AM +1000, Herbert Xu wrote:
 On Thu, Jun 04, 2009 at 08:04:56PM -0400, Neil Horman wrote:
 
  Not sure what to do about this.  The intent is to provide the external 
  reference
  to the fips_enabled flag (which is either defined as an extern in or 
  #defined to
  0 dependent on CONFIG_CRYPTO_FIPS).  I can cut'n'paste the code block from 
  the
  include file and put it in here, but that seems like a worse solution to me.
  Let me know your thoughts, and I can change this accordingly.
 
 You can put the fips flag into its own header file, e.g., linux/fips.h.
 

Ok, that seems like a reasonable idea.  new patch below
Change Notes:

1) Moved fips_enabled flag into its own header, included in all the appropriate
places

2) removed flags field from entropy store, keying continuous test on the
non-null status of the last_data pointer


FIPS-140 requires that all random number generators implement continuous self
tests in which each extracted block of data is compared against the last block
for repetition.  The ansi_cprng implements such a test, but it would be nice if
the hw rng's did the same thing.  Obviously its not something thats always
needed, but it seems like it would be a nice feature to have on occasion. I've
written the below patch which allows individual entropy stores to be flagged as
desiring a continuous test to be run on them as is extracted.  By default this
option is off, but is enabled in the event that fips mode is selected during
bootup.

Neil

Signed-off-by: Neil Horman nhor...@tuxdriver.com


crypto/internal.h |7 +--
drivers/char/random.c |   14 ++
include/linux/fips.h  |   10 ++
3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/crypto/internal.h b/crypto/internal.h
index fc76e1f..769b713 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -25,12 +25,7 @@
 #include linux/notifier.h
 #include linux/rwsem.h
 #include linux/slab.h
-
-#ifdef CONFIG_CRYPTO_FIPS
-extern int fips_enabled;
-#else
-#define fips_enabled 0
-#endif
+#include linux/fips.h
 
 /* Crypto notification events. */
 enum {
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 8c74448..d8a9255 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -240,6 +240,7 @@
 #include linux/spinlock.h
 #include linux/percpu.h
 #include linux/cryptohash.h
+#include linux/fips.h
 
 #ifdef CONFIG_GENERIC_HARDIRQS
 # include linux/irq.h
@@ -413,6 +414,7 @@ struct entropy_store {
unsigned add_ptr;
int entropy_count;
int input_rotate;
+   __u8 *last_data;
 };
 
 static __u32 input_pool_data[INPUT_POOL_WORDS];
@@ -852,12 +854,21 @@ static ssize_t extract_entropy(struct entropy_store *r, 
void *buf,
 {
ssize_t ret = 0, i;
__u8 tmp[EXTRACT_SIZE];
+   unsigned long flags;
 
xfer_secondary_pool(r, nbytes);
nbytes = account(r, nbytes, min, reserved);
 
while (nbytes) {
extract_buf(r, tmp);
+
+   if (r-last_data) {
+   spin_lock_irqsave(r-lock, flags);
+   if (!memcmp(tmp, r-last_data, EXTRACT_SIZE))
+   panic(Hardware RNG duplicated output!\n);
+   memcpy(r-last_data, tmp, EXTRACT_SIZE);
+   spin_unlock_irqrestore(r-lock, flags);
+   }
i = min_t(int, nbytes, EXTRACT_SIZE);
memcpy(buf, tmp, i);
nbytes -= i;
@@ -940,6 +951,9 @@ static void init_std_data(struct entropy_store *r)
now = ktime_get_real();
mix_pool_bytes(r, now, sizeof(now));
mix_pool_bytes(r, utsname(), sizeof(*(utsname(;
+   /* Enable continuous test in fips mode */
+   if (fips_enabled)
+   r-last_data = kmalloc(EXTRACT_SIZE, GFP_KERNEL);
 }
 
 static int rand_initialize(void)
diff --git a/include/linux/fips.h b/include/linux/fips.h
new file mode 100644
index 000..f8fb07b
--- /dev/null
+++ b/include/linux/fips.h
@@ -0,0 +1,10 @@
+#ifndef _FIPS_H
+#define _FIPS_H
+
+#ifdef CONFIG_CRYPTO_FIPS
+extern int fips_enabled;
+#else
+#define fips_enabled 0
+#endif
+
+#endif
 Cheers,
 -- 
 Visit Openswan at http://www.openswan.org/
 Email: Herbert Xu ~{PmVHI~} herb...@gondor.apana.org.au
 Home Page: http://gondor.apana.org.au/~herbert/
 PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] crypto: add buffer overflow checks to testmgr

[Neil Horman]

On Fri, May 29, 2009 at 11:32:54AM -0400, Jarod Wilson wrote:
 At present, its entirely possible to add a test vector to testmgr with
 an input longer than a page in length w/o specifying a .np option, and
 overflow the page of memory allocated to {a,}xbuf[0], silently
 corrupting memory. I know, because I've accidentally done it. :)
 
 While this doesn't currently happen in practice w/the existing code,
 due to all !np vectors being less than a 4k page in length (and the
 page allocation loop often returns contiguous pages anyway), explicit
 checks or a way to remove the 4k limit would be a good idea.
 
 A few ways to fix and/or work around this:
 
 1) allocate some larger guaranteed contiguous buffers using
 __get_free_pages() or kmalloc and use them in the !np case
 
 2) catch the  PAGE_SIZE  !np case and then do things similar to how
 they are done in the np case
 
 3) catch the  PAGE_SIZE  !np case and simply exit with an error
 
 Since there currently aren't any test vectors that are actually larger
 than a page and not tagged np, option 1 seems like a waste of memory
 and option 2 sounds like unnecessary complexity, so I'd offer up
 option 3 as the most viable alternative right now.
 
 Signed-off-by: Jarod Wilson ja...@redhat.com
 

Given the rate at which test vectors are added to the testmgr, and the
likelyhood one will be over a page in size, I think this is probably the best
way forward.  Its saves memory/complexity until for now, and catches anyone
trying to exceed the current 1 page size, at which point we can spend the time
to modify the testmanager to make use of scatter/gather chains to handle the
longer vectors.

Neil

Acked-by: Neil Horman nhor...@tuxdriver.com

 ---
  crypto/testmgr.c |   34 ++
  1 files changed, 34 insertions(+), 0 deletions(-)
 
 diff --git a/crypto/testmgr.c b/crypto/testmgr.c
 index 376ea88..9483a2b 100644
 --- a/crypto/testmgr.c
 +++ b/crypto/testmgr.c
 @@ -185,6 +185,13 @@ static int test_hash(struct crypto_ahash *tfm, struct 
 hash_testvec *template,
  
   hash_buff = xbuf[0];
  
 + if (template[i].psize  PAGE_SIZE) {
 + printk(KERN_ERR alg: hash: psize %u larger than 
 +contiguous buffer space\n, template[i].psize);
 + ret = -EOVERFLOW;
 + goto out;
 + }
 +
   memcpy(hash_buff, template[i].plaintext, template[i].psize);
   sg_init_one(sg[0], hash_buff, template[i].psize);
  
 @@ -357,6 +364,16 @@ static int test_aead(struct crypto_aead *tfm, int enc,
   input = xbuf[0];
   assoc = axbuf[0];
  
 + if (template[i].ilen  PAGE_SIZE ||
 + template[i].alen  PAGE_SIZE) {
 + printk(KERN_ERR alg: aead: input larger than 
 +contiguous buffer space (ilen: %u, 
 +alen: %u)\n,
 +template[i].ilen, template[i].alen);
 + ret = -EOVERFLOW;
 + goto out;
 + }
 +
   memcpy(input, template[i].input, template[i].ilen);
   memcpy(assoc, template[i].assoc, template[i].alen);
   if (template[i].iv)
 @@ -651,6 +668,14 @@ static int test_cipher(struct crypto_cipher *tfm, int 
 enc,
   j++;
  
   data = xbuf[0];
 +
 + if (template[i].ilen  PAGE_SIZE) {
 + printk(KERN_ERR alg: cipher: ilen %u larger than 
 +contiguous buffer space\n, template[i].ilen);
 + ret = -EOVERFLOW;
 + goto out;
 + }
 +
   memcpy(data, template[i].input, template[i].ilen);
  
   crypto_cipher_clear_flags(tfm, ~0);
 @@ -742,6 +767,15 @@ static int test_skcipher(struct crypto_ablkcipher *tfm, 
 int enc,
   j++;
  
   data = xbuf[0];
 +
 + if (template[i].ilen  PAGE_SIZE) {
 + printk(KERN_ERR alg: skcipher: ilen %u larger 
 +than contiguous buffer space\n,
 +template[i].ilen);
 + ret = -EOVERFLOW;
 + goto out;
 + }
 +
   memcpy(data, template[i].input, template[i].ilen);
  
   crypto_ablkcipher_clear_flags(tfm, ~0);
 
 -- 
 Jarod Wilson
 ja...@redhat.com
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: tcrypt: add option to not exit on success

[Neil Horman]

On Wed, May 13, 2009 at 11:30:50AM +1000, Herbert Xu wrote:
 On Tue, May 12, 2009 at 08:37:27PM -0400, Neil Horman wrote:
 
   Would there be any objections to dropping the noexit parameter
   entirely and just making its behavior the default? It would make
   all users regardless of fips mode notice failures more readily.
   
  I think thats a fine idea.  Theres no reason that a user of the tcrypt 
  module
  can't manually rmmod it when the testing is done.  Doing it that way just 
  seems
  more sane to me to begin with anyway.
 
 No, tcrypt is only a relic for correctness testing.  Its main
 purpose these days is for speed testing.  Having to rmmod it
 is silly.
 
 There's really no need to load tcrypt for correctness testing
 anymore.
 

Not really sure I agree with the logic here.  I agree that its pretty clear that
its major value is for quickly testing all the algorithms in a system, but
universally failing the loading of the module simply to save a few milliseconds
seems like a poor choice.  In so doing you create an alias effect, as jarod
noted between a non-existent module and a module that failed to load.  The
aliasing can be resolved, if you want to parse dmesg, but if speed is the issue
at hand, that parsing is a significant impact.  If you allow the module to load
properly, then for the cost of an rmmod, you can tell simply from the exit code
of modprobe:
1) If the module was found 
2) If the tests passed

And if the rmmod is simply to expensive for whatever reason, then for the cost
of a few k of ram taken up by the module, you can choose not to unload it.

Of course, if tcrypt is really as much of a relic as you say, perhaps that is an
argument for removing the module entirely.  Perhaps the testmgr interface could
be exported to userspace and the tcrypt tests be packaged as a userspace suite.

Regards
Neil

 Cheers,
 -- 
 Visit Openswan at http://www.openswan.org/
 Email: Herbert Xu ~{PmVHI~} herb...@gondor.apana.org.au
 Home Page: http://gondor.apana.org.au/~herbert/
 PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: tcrypt: add option to not exit on success

[Neil Horman]

On Wed, May 13, 2009 at 11:27:52PM +1000, Herbert Xu wrote:
 On Wed, May 13, 2009 at 09:12:46AM -0400, Jarod Wilson wrote:
  
  Hm... FIPS has the requirement that we test all algs before we use any
  algs, self-tests on demand before first use for each alg is
  insufficient. At first blush, I'm not seeing how we ensure this
  happens. How can we trigger a cbc(des3_ede) self-test from userspace?
  I see that modprobe'ing des.ko runs the base des and des3_ede
  self-tests, but modprobe'ing cbc.ko doesn't lead to any self-tests
  being run.
 
 Once we have a user-space interface crypto API you will be able
 to instantiate any given algorithm.
 
Thats a good idea.  Jarod, didn't you create a generic netlink socket family
module that created just such an interface for testing purposes?  That might be
worth polishing and submitting to provide that user space crypto api

Neil

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

Re: [PATCH 1/2 v2] crypto: mark algs allowed in fips mode

[Neil Horman]

On Mon, May 11, 2009 at 09:52:43AM -0400, Jarod Wilson wrote:
 Set the fips_allowed flag in testmgr.c's alg_test_descs[] for algs
 that are allowed to be used when in fips mode.
 
 One caveat: des isn't actually allowed anymore, but des (and thus also
 ecb(des)) has to be permitted, because disallowing them results in
 des3_ede being unable to properly register (see des module init func).
 
 Also, crc32 isn't technically on the fips approved list, but I think
 it gets used in various places that necessitate it being allowed.
 
 This list is based on
 http://csrc.nist.gov/groups/STM/cavp/index.html
 
 Important note: allowed/approved here does NOT mean validated, just
 that its an alg that *could* be validated.
 
 Resending with properly updated patch v2 tag.
 
 Signed-off-by: Jarod Wilson ja...@redhat.com
 
Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH 2/2 v2] crypto: skip algs not flagged fips_allowed in fips mode

[Neil Horman]

On Mon, May 11, 2009 at 09:53:06AM -0400, Jarod Wilson wrote:
 Because all fips-allowed algorithms must be self-tested before they
 can be used, they will all have entries in testmgr.c's alg_test_descs[].
 Skip self-tests for any algs not flagged as fips_approved and return
 -EINVAL when in fips mode.
 
 Resending with properly updated patch v2 tag.
 
 Signed-off-by: Jarod Wilson ja...@redhat.com
 
Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH v2] crypto: don't raise alarm for no ctr(aes) tests

[Neil Horman]

On Thu, Apr 30, 2009 at 05:13:25PM -0400, Jarod Wilson wrote:
 On Wednesday 29 April 2009 08:46:47 Jarod Wilson wrote:
  On Wednesday 29 April 2009 06:50:35 Neil Horman wrote:
   On Tue, Apr 28, 2009 at 09:18:22PM -0400, Jarod Wilson wrote:
Per the NIST AESAVS document, Appendix A[1], it isn't possible to
have automated self-tests for counter-mode AES, but people are
misled to believe something is wrong by the message that says there
is no test for ctr(aes). Simply suppress all 'no test for ctr(aes*'
messages if fips_enabled is set to avoid confusion.

Dependent on earlier patch 'crypto: catch base cipher self-test
failures in fips mode', which adds the test_done label.

[1] http://csrc.nist.gov/groups/STM/cavp/documents/aes/AESAVS.pdf
 [...]
   From the way I read the document, anything operating in a counter mode 
   will have
   an unpredictable output (given the counter operation isn't specified).  
   While
   the above works, I'm not sure that it fully covers the various ccm modes
   available (ccm_base and rfc4309).
  
  I believe Appendix A only applies for straight up counter-mode aes,
  ccm_base and rfc4309 actually have well-defined counter operations.
  We've already got self-tests for ccm(aes) and a pending patch for
  rfc4309(ccm(aes), and since they don't start w/'ctr(aes', they
  wouldn't be caught by that (admittedly hacky) check even if we
  didn't have test vectors for them.
  
   Perhaps instead it would be better to add a
   TFM mask flag indicating that the selected transform included a 
   unpredictable
   component or counter input (marking the alg as being unsuitable for 
   automatic
   testing without knoweldge of the inner workings of that counter.  Then 
   you could
   just test for that flag?
  
  Yeah, I thought about a flag too, but it seemed potentially a lot of
  overhead for what might well be restricted to ctr(aes*). It might've
  been relevant for ctr(des3_ede) or ctr(des), but they're not on the
  fips approved algo/mode list, so I took the easy way out. I'm game to
  go the flag route if need be though.
 
 Neil and I talked a bit more off-list about the best approach to take, and
 after a bit of trial and error, we came up with the idea to simply add an
 'untestable' flag to the alg_test_desc struct, a corresponding entry for
 ctr(aes) in alg_test_descs, and a hook to check for untestable algs in
 alg_find_test().
 
 Works well enough in local testing, eliminates the use of strncmp, and
 results in far more granular control over marking which algs are
 explicitly untestable and shouldn't have warnings printed for. At the
 moment, I've gone for suppressing the warnings regardless of whether
 we're in fips mode or not, but adding a different warning (er, info)
 message in the untestable case works too, if that's preferred. The
 errnos used seem appropriate, but I might have missed more appropriate
 ones somewhere.
 
 nb: this patch applies atop my earlier '[PATCH v2] crypto: catch base
 cipher self-test failures in fips mode'.
 
 Signed-off-by: Jarod Wilson ja...@redhat.com
 
 ---
  crypto/testmgr.c |   21 +++--
  1 files changed, 19 insertions(+), 2 deletions(-)
 
 diff --git a/crypto/testmgr.c b/crypto/testmgr.c
 index f39c148..b78278c 100644
 --- a/crypto/testmgr.c
 +++ b/crypto/testmgr.c
 @@ -94,6 +94,7 @@ struct alg_test_desc {
   const char *alg;
   int (*test)(const struct alg_test_desc *desc, const char *driver,
   u32 type, u32 mask);
 + int untestable;
  
   union {
   struct aead_test_suite aead;
 @@ -1518,6 +1519,13 @@ static const struct alg_test_desc alg_test_descs[] = {
   }
   }
   }, {
 + /*
 +  * Automated ctr(aes) tests aren't possible per Appendix A of
 +  * http://csrc.nist.gov/groups/STM/cavp/documents/aes/AESAVS.pdf
 +  */
 + .alg = ctr(aes),
 + .untestable = 1,
 + }, {
   .alg = cts(cbc(aes)),
   .test = alg_test_skcipher,
   .suite = {
 @@ -2198,10 +2206,13 @@ static int alg_find_test(const char *alg)
   continue;
   }
  
 + if (alg_test_descs[i].untestable)
 + return -ENODATA;
 +
   return i;
   }
  
 - return -1;
 + return -ENOSYS;
  }
  
  int alg_test(const char *driver, const char *alg, u32 type, u32 mask)
 @@ -2237,7 +2248,13 @@ test_done:
   return rc;
  
  notest:
 - printk(KERN_INFO alg: No test for %s (%s)\n, alg, driver);
 + switch (i) {
 + case -ENODATA:
 + break;
 + case -ENOSYS:
 + default:
 + printk(KERN_INFO alg: No test for %s (%s)\n, alg, driver);
 + }
   return 0;
  }
  EXPORT_SYMBOL_GPL(alg_test);
 
 
 -- 
 Jarod Wilson
 ja...@redhat.com
 

Acked-by: Neil Horman nhor...@tuxdriver.com

--
To unsubscribe from this list: send the line

Re: [PATCH] crypto: don't raise alarm for no ctr(aes*) tests in fips mode

[Neil Horman]

On Tue, Apr 28, 2009 at 09:18:22PM -0400, Jarod Wilson wrote:
 Per the NIST AESAVS document, Appendix A[1], it isn't possible to
 have automated self-tests for counter-mode AES, but people are
 misled to believe something is wrong by the message that says there
 is no test for ctr(aes). Simply suppress all 'no test for ctr(aes*'
 messages if fips_enabled is set to avoid confusion.
 
 Dependent on earlier patch 'crypto: catch base cipher self-test
 failures in fips mode', which adds the test_done label.
 
 [1] http://csrc.nist.gov/groups/STM/cavp/documents/aes/AESAVS.pdf
 
 Signed-off-by: Jarod Wilson ja...@redhat.com
 
 ---
  crypto/testmgr.c |   11 +++
  1 files changed, 11 insertions(+), 0 deletions(-)
 
 diff --git a/crypto/testmgr.c b/crypto/testmgr.c
 index 5a50416..39ffa69 100644
 --- a/crypto/testmgr.c
 +++ b/crypto/testmgr.c
 @@ -2134,6 +2134,17 @@ int alg_test(const char *driver, const char *alg, u32 
 type, u32 mask)
 type, mask);
   goto test_done;
  notest:
 + /*
 +  * Per NIST AESAVS[1], it isn't possible to have automated self-tests
 +  * for counter mode aes vectors, they have to be covered by ecb mode
 +  * and code inspection. The ecb mode tests are trigger above in the
 +  * CRYPTO_ALG_TYPE_CIPHER section. Suppress warnings about missing
 +  * ctr tests if we're in fips mode to avoid confusion.
 +  *
 +  * [1] http://csrc.nist.gov/groups/STM/cavp/documents/aes/AESAVS.pdf
 +  */
 + if (fips_enabled  !strncmp(alg, ctr(aes, 7))
 + goto test_done;
   printk(KERN_INFO alg: No test for %s (%s)\n, alg, driver);
  test_done:
   if (fips_enabled  rc)
 
From the way I read the document, anything operating in a counter mode will 
have
an unpredictable output (given the counter operation isn't specified).  While
the above works, I'm not sure that it fully covers the various ccm modes
available (ccm_base and rfc4309).  Perhaps instead it would be better to add a
TFM mask flag indicating that the selected transform included a unpredictable
component or counter input (marking the alg as being unsuitable for automatic
testing without knoweldge of the inner workings of that counter.  Then you could
just test for that flag?

Neil

 -- 
 Jarod Wilson
 ja...@redhat.com
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: print self-test pass notices in fips mode

[Neil Horman]

On Tue, Apr 28, 2009 at 09:21:35PM -0400, Jarod Wilson wrote:
 According to our FIPS CAVS testing lab guru, when we're in fips mode,
 we *must* print out notices of successful self-test completion for
 every alg to be compliant.
 
 Dependent on patch 'crypto: catch base cipher self-test failures in
 fips mode', which adds the test_done label.
 
 Signed-off-by: Jarod Wilson ja...@redhat.com
 
 ---
  crypto/testmgr.c |4 
  1 files changed, 4 insertions(+), 0 deletions(-)
 
 diff --git a/crypto/testmgr.c b/crypto/testmgr.c
 index 39ffa69..d0cc85c 100644
 --- a/crypto/testmgr.c
 +++ b/crypto/testmgr.c
 @@ -2149,6 +2149,10 @@ notest:
  test_done:
   if (fips_enabled  rc)
   panic(%s: %s alg self test failed in fips mode!\n, driver, 
 alg);
 + /* fips mode requires we print out self-test success notices */
 + if (fips_enabled  !rc  strncmp(alg, ctr(aes, 7))
 + printk(KERN_INFO alg: self-tests for %s (%s) passed\n,
 +driver, alg);
   return rc;
  }
  EXPORT_SYMBOL_GPL(alg_test);
 
 -- 
 Jarod Wilson
 ja...@redhat.com
 

Acked-by: Neil Horman nhor...@tuxdriver.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] add infrastructure for ansi_cprng self-tests

[Neil Horman]

On Tue, Apr 28, 2009 at 09:28:13PM -0400, Jarod Wilson wrote:
 Add some necessary infrastructure to make it possible to run
 self-tests for ansi_cprng. The bits are likely very specific
 to the ANSI X9.31 CPRNG in AES mode, and thus perhaps should
 be named more specifically if/when we grow additional CPRNG
 support...
 
 Successfully tested against the cryptodev-2.6 tree and a
 Red Hat Enterprise Linux 5.x kernel with the follow-on
 patch that adds the actual test vectors.
 
 Signed-off-by: Jarod Wilson ja...@redhat.com
 
Thanks Jarod!
Acked-by: Neil Horman nhor...@tuxdriver.com

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

Re: [PATCH 2/2] add ansi_cprng test vectors

[Neil Horman]

On Tue, Apr 28, 2009 at 09:32:10PM -0400, Jarod Wilson wrote:
 Add ANSI X9.31 Continuous Pseudo-Random Number Generator (AES mode),
 aka 'ansi_cprng' test vectors, taken from Appendix B.2.9 and B.2.10
 of the NIST RNGVS document, found here:
 http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf
 
 Successfully tested against both the cryptodev-2.6 tree and a Red
 Hat Enterprise Linux 5.4 kernel, via 'modprobe tcrypt mode=150'.
 
 The selection of 150 was semi-arbitrary, didn't seem like it should
 go any place in particular, so I started a new range for rng tests.
 
 Signed-off-by: Jarod Wilson ja...@redhat.com
 
Acked-by: Neil Horman nhor...@tuxdriver.com
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add self-tests for rfc4309(ccm(aes))

[Neil Horman]

On Thu, Apr 09, 2009 at 03:16:53PM -0400, Jarod Wilson wrote:
 On Thursday 09 April 2009 14:52:04 Neil Horman wrote:
  On Thu, Apr 09, 2009 at 02:34:59PM -0400, Jarod Wilson wrote:
   Patch is against current cryptodev-2.6 tree, successfully tested via
   'modprobe tcrypt type=45'. The number of test vectors might be a bit
   excessive, but I tried to hit a wide range of combinations of varying
   key sizes, associate data lengths, input lengths and pass/fail.
   
   Signed-off-by: Jarod Wilson ja...@redhat.com
   

  snip
  
   +/*
   + * rfc4309 says section 8 contains test vectors, only, it doesn't, and 
   NIST
   + * Special Publication 800-38C's test vectors use nonce lengths our 
   rfc4309
   + * implementation doesn't support. The following are taken from fips cavs
   + * fax files on hand at Red Hat.
   + *
   + * nb: actual key lengths are (klen - 3), the last 3 bytes are actually
   + * part of the nonce which combine w/the iv, but need to be input this 
   way.
   + */
  
  RFC4309 section 8 actually says the test vectors you can use are here:
  http://www.ietf.org/rfc/rfc3610.txt
  in RFC3610 section 8.
 
 Oh, I'm dense, didn't correctly parse that 4309 was referring back to 3610
 for the actual test vectors. I'll see what I can do with those...
 
Its easy to miss.  It referrs to the RFC in an endnote by reference.

  I don't think theres anything wrong with the vectors
  your're using below, but you may want to add the vectors from 3610 just to
  imrpove the testing.
 
 I think I'd drop some of the ones in the initial patch in favor of adding
 some from 3610, rather than simply adding more. The coverage is already
 pretty good, increasing the number of vectors shouldn't really be necessary,
 but it would definitely be nice to have vectors that are already publicly
 published and verified.
 
ACK to that.

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

Re: [PATCH][trivial] crypto: tcrypt - reduce stack size

[Neil Horman]

On Wed, Feb 25, 2009 at 02:48:19PM +0100, Frank Seidel wrote:
 From: Frank Seidel fr...@f-seidel.de
 
 Applying kernel janitors todos (printk calls need KERN_*
 constants on linebeginnings, reduce stack footprint where
 possible) to tcrypts test_hash_speed (where stacks
 memory footprint was very high (on i386 1184 bytes to
 292 now).
 
 Signed-off-by: Frank Seidel fr...@f-seidel.de
 ---
  
   if (IS_ERR(tfm)) {
 - printk(failed to load transform for %s: %ld\n, algo,
 + printk(KERN_ERR failed to load transform for %s: %ld\n, algo,
  PTR_ERR(tfm));
 + kfree(output);
   return;
Its just a stylistic nit, but I think I'd rather see this be a goto to the kfree
at the end of the function.  You can move the out label down to just the kfree,
and rename the current out usage to out_free_tfm.  I think if you're going to
add the kfree at the bottom, you might as well make use of it for all cases.

If you change that, I'll ack this.

Thanks  Regards
Neil

   }
  
   desc.tfm = tfm;
   desc.flags = 0;
  
 - if (crypto_hash_digestsize(tfm)  sizeof(output)) {
 - printk(digestsize(%u)  outputbuffer(%zu)\n,
 -crypto_hash_digestsize(tfm), sizeof(output));
 + if (crypto_hash_digestsize(tfm)  output_size) {
 + printk(KERN_ERR digestsize(%u)  outputbuffer(%zu)\n,
 +crypto_hash_digestsize(tfm), output_size);
   goto out;
   }
  
 @@ -427,12 +435,14 @@ static void test_hash_speed(const char *
  
   for (i = 0; speed[i].blen != 0; i++) {
   if (speed[i].blen  TVMEMSIZE * PAGE_SIZE) {
 - printk(template (%u) too big for tvmem (%lu)\n,
 + printk(KERN_ERR
 +template (%u) too big for tvmem (%lu)\n,
  speed[i].blen, TVMEMSIZE * PAGE_SIZE);
   goto out;
   }
  
 - printk(test%3u (%5u byte blocks,%5u bytes per update,%4u 
 updates): ,
 + printk(KERN_INFO test%3u 
 +(%5u byte blocks,%5u bytes per update,%4u updates): ,
  i, speed[i].blen, speed[i].plen, speed[i].blen / 
 speed[i].plen);
  
   if (sec)
 @@ -443,13 +453,14 @@ static void test_hash_speed(const char *
  speed[i].plen, output);
  
   if (ret) {
 - printk(hashing failed ret=%d\n, ret);
 + printk(KERN_ERR hashing failed ret=%d\n, ret);
   break;
   }
   }
  
  out:
   crypto_free_hash(tfm);
 + kfree(output);
  }
  
  static void test_available(void)
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3][trivial] crypto: tcrypt - reduce stack size

[Neil Horman]

On Wed, Feb 25, 2009 at 03:53:14PM +0100, Frank Seidel wrote:
 From: Frank Seidel fr...@f-seidel.de
 
 Applying kernel janitors todos (printk calls need KERN_*
 constants on linebeginnings, reduce stack footprint where
 possible) to tcrypts test_hash_speed (where stacks
 memory footprint was very high (on i386 1184 bytes to
 160 now).
 
 Signed-off-by: Frank Seidel fr...@f-seidel.de
Looks good, thanks Frank
Acked-by: Neil Horman nhor...@tuxdriver.com

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

[PATCH] crypto: Force panic on continuous CPRNG test failure when in FIPS mode

[Neil Horman]

FIPS 140-2 specifies that all access to various cryptographic modules be
prevented in the event that any of the provided self tests fail on the various
implemented algorithms. The way this is currently done is by simply panicing the
box.  We do this already for the various alg tests in testmgr.c, we should do it
in the case of a failure for the continuous test in the CPRNG as well.  This
patch implements that change

Signed-off-by: Neil Horman nhor...@tuxdriver.com

diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 0fac8ff..7eef5be 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -132,10 +132,20 @@ static int _get_more_prng_bytes(struct prng_context *ctx)
 */
if (!memcmp(ctx-rand_data, ctx-last_rand_data,
DEFAULT_BLK_SZ)) {
-   printk(KERN_ERR
-   ctx %p Failed repetition check!\n,
-   ctx);
-   ctx-flags |= PRNG_NEED_RESET;
+   if (fips_enabled) {
+   /* FIPS 140-2 requires that we disable
+* further use of crypto code if we fail
+* this test,  easiest way to do that
+* is panic the box
+*/
+   panic(cprng %p failed continuity test,
+   ctx);  
+   } else {
+   printk(KERN_ERR
+   ctx %p Failed repetition 
check!\n,
+   ctx);
+   ctx-flags |= PRNG_NEED_RESET;
+   }
return -EINVAL;
}
memcpy(ctx-last_rand_data, ctx-rand_data,
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: force reset of cprng on allocation

[Neil Horman]

pseudo RNGs provide predictable outputs based on input parateters {key, V, DT},
the idea behind them is that only the user should know what the inputs are.
While its nice to have default known values for testing purposes, it seems
dangerous to allow the use of those default values without some sort of safety
measure in place, lest an attacker easily guess the output of the cprng.  This
patch forces the NEED_RESET flag on when allocating a cprng context, so that any
user is forced to reseed it before use.  The defaults can still be used for
testing, but this will prevent their inadvertent use, and be more secure.
   
Signed-off-by: Neil Horman nhor...@redhat.com

ansi_cprng.c |   11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index 7eef5be..d9c3971 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -348,7 +348,16 @@ static int cprng_init(struct crypto_tfm *tfm)
 
spin_lock_init(ctx-prng_lock);
 
-   return reset_prng_context(ctx, NULL, DEFAULT_PRNG_KSZ, NULL, NULL);
+   if (reset_prng_context(ctx, NULL, DEFAULT_PRNG_KSZ, NULL, NULL)  0)
+   return -EINVAL;
+
+   /*
+* after allocation, we should always force the user to reset
+* so they don't inadvertently use the insecure default values
+* without specifying them intentially
+*/
+   ctx-flags |= PRNG_NEED_RESET;
+   return 0;
 }
 
 static void cprng_exit(struct crypto_tfm *tfm)
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: fix handling of ccm vectors with null assoc data

[Neil Horman]

On Wed, Jan 21, 2009 at 04:41:01PM -0500, Jarod Wilson wrote:
 Its a valid use case to have null associated data in a ccm vector, but
 this case isn't being handled properly right now.
 
 The following ccm decryption/verification test vector, using the
 rfc4309 implementation regularly triggers a panic, as will any
 other vector with null assoc data:
 
 * key: ab2f8a74b71cd2b1ff802e487d82f8b9
 * iv: c6fb7d800d13abd8a6b2d8
 * Associated Data: [NULL]
 * Tag Length: 8
 * input: d5e8939fc7892e2b
 
 The resulting panic looks like so:
 
 Unable to handle kernel paging request at 810064ddaec0 RIP: 
  [8864c4d7] :ccm:get_data_to_compute+0x1a6/0x1d6
 PGD 8063 PUD 0 
 Oops: 0002 [1] SMP 
 last sysfs file: /module/libata/version
 CPU 0
 Modules linked in: crypto_tester_kmod(U) seqiv krng ansi_cprng chainiv rng 
 ctr aes_generic aes_x86_64 ccm cryptomgr testmgr_cipher testmgr aead 
 crypto_blkcipher crypto_a
 lgapi des ipv6 xfrm_nalgo crypto_api autofs4 hidp l2cap bluetooth nfs lockd 
 fscache nfs_acl sunrpc ip_conntrack_netbios_ns ipt_REJECT xt_state 
 ip_conntrack nfnetlink xt_
 tcpudp iptable_filter ip_tables x_tables dm_mirror dm_log dm_multipath 
 scsi_dh dm_mod video hwmon backlight sbs i2c_ec button battery asus_acpi 
 acpi_memhotplug ac lp sg 
 snd_intel8x0 snd_ac97_codec ac97_bus snd_seq_dummy snd_seq_oss joydev 
 snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss ide_cd 
 snd_pcm floppy parport_p
 c shpchp e752x_edac snd_timer e1000 i2c_i801 edac_mc snd soundcore 
 snd_page_alloc i2c_core cdrom parport serio_raw pcspkr ata_piix libata sd_mod 
 scsi_mod ext3 jbd uhci_h
 cd ohci_hcd ehci_hcd
 Pid: 12844, comm: crypto-tester Tainted: G  2.6.18-128.el5.fips1 #1
 RIP: 0010:[8864c4d7]  [8864c4d7] 
 :ccm:get_data_to_compute+0x1a6/0x1d6
 RSP: 0018:8100134434e8  EFLAGS: 00010246
 RAX:  RBX: 8100104898b0 RCX: ab6aea10
 RDX: 0010 RSI: 8100104898c0 RDI: 810064ddaec0
 RBP:  R08: 8100104898b0 R09: 
 R10: 8100103bac84 R11: 8100104898b0 R12: 810010489858
 R13: 8100104898b0 R14: 8100103bac00 R15: 
 FS:  2ab881adfd30() GS:803ac000() knlGS:
 CS:  0010 DS:  ES:  CR0: 8005003b
 CR2: 810064ddaec0 CR3: 12a88000 CR4: 06e0
 Process crypto-tester (pid: 12844, threadinfo 810013442000, task 
 81003d165860)
 Stack:  8100103bac00 8100104898e8 8100134436f8 
   8100104898b0  810010489858
   8100103bac00 8100134436f8 8864c634
 Call Trace:
  [8864c634] :ccm:crypto_ccm_auth+0x12d/0x140
  [8864cf73] :ccm:crypto_ccm_decrypt+0x161/0x23a
  [88633643] :crypto_tester_kmod:cavs_test_rfc4309_ccm+0x4a5/0x559
 [...]
 
 The above is from a RHEL5-based kernel, but upstream is susceptible too.
 
 The fix is trivial: in crypto/ccm.c:crypto_ccm_auth(), pctx-ilen contains
 whatever was in memory when pctx was allocated if assoclen is 0. The tested
 fix is to simply add an else clause setting pctx-ilen to 0 for the
 assoclen == 0 case, so that get_data_to_compute() doesn't try doing
 things its not supposed to.
 
 Signed-off-by: Jarod Wilson ja...@redhat.com
 


This looks good to me.  Thanks Jarod!
Acked-by: Neil Horman nhor...@tuxdriver.com

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