Re: [PATCH net-next 3/4] tls: kernel TLS support

2017-05-26 Thread Eric Dumazet 
On Fri, 2017-05-26 at 11:18 -0400, David Miller wrote:
> From: Eric Dumazet <eric.duma...@gmail.com>
> Date: Fri, 26 May 2017 07:16:59 -0700
> 
> > On Wed, 2017-05-24 at 09:27 -0700, Dave Watson wrote:
> >> Software implementation of transport layer security, implemented using ULP
> >> infrastructure.  tcp proto_ops are replaced with tls equivalents of 
> >> sendmsg and
> >> sendpage.
> > 
> > ...
> > 
> >> +
> >> +int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> >> +{
> > ...
> >> +
> >> +  lock_sock(sk);
> >> +
> >> +  /* Only one writer at a time is allowed */
> >> +  if (sk->sk_write_pending)
> >> +  return -EBUSY;
> > 
> > Ouch...
> 
> Well, as I understand it, it is the same restriction userspace must
> itself enforce either in the application or in the SSL library.

The problem here is to lock_sock(sk), then returning without releasing
the socket.

Some basic lock imbalance really.

Re: [PATCH net-next 3/4] tls: kernel TLS support

2017-05-26 Thread Eric Dumazet 
On Wed, 2017-05-24 at 09:27 -0700, Dave Watson wrote:
> Software implementation of transport layer security, implemented using ULP
> infrastructure.  tcp proto_ops are replaced with tls equivalents of sendmsg 
> and
> sendpage.

...

> +
> +int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> +{
...
> +
> + lock_sock(sk);
> +
> + /* Only one writer at a time is allowed */
> + if (sk->sk_write_pending)
> + return -EBUSY;

Ouch...

Re: [kernel-hardening] Re: HalfSipHash Acceptable Usage

2016-12-21 Thread Eric Dumazet 
On Wed, 2016-12-21 at 11:39 -0500, Rik van Riel wrote:

> Does anybody still have a P4?
> 
> If they do, they're probably better off replacing
> it with an Atom. The reduced power bills will pay
> for replacing that P4 within a year or two.

Well, maybe they have millions of units to replace.

> 
> In short, I am not sure how important the P4
> performance numbers are, especially if we can
> improve security for everybody else...

Worth adding that the ISN or syncookie generation are less than 10% of
the actual cost of handling a problematic (having to generate ISN or
syncookie) TCP packet anyway.

So we are talking of minors potential impact for '2000-era' cpus.

Definitely I vote for using SipHash in TCP ASAP.


--
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: HalfSipHash Acceptable Usage

2016-12-21 Thread Eric Dumazet 
On Wed, 2016-12-21 at 15:42 +0100, Jason A. Donenfeld wrote:
> Hi Eric,
> 
> I computed performance numbers for both 32-bit and 64-bit using the
> actual functions in which talking about replacing MD5 with SipHash.
> The basic harness is here [1] if you're curious. SipHash was a pretty
> clear winner for both cases.
> 
> x86_64:
> [1.714302] secure_tcpv6_sequence_number_md5# cycles: 102373398
> [1.747685] secure_tcp_sequence_number_md5# cycles: 92042258
> [1.773522] secure_tcpv6_sequence_number_siphash# cycles: 70786533
> [1.798701] secure_tcp_sequence_number_siphash# cycles: 68941043
> 
> x86:
> [1.635749] secure_tcpv6_sequence_number_md5# cycles: 106016335
> [1.670259] secure_tcp_sequence_number_md5# cycles: 95670512
> [1.708387] secure_tcpv6_sequence_number_siphash# cycles: 105988635
> [1.740264] secure_tcp_sequence_number_siphash# cycles: 88225395
> 
> >>> 102373398 > 70786533
> True
> >>> 92042258 > 68941043
> True
> >>> 106016335 > 105988635
> True
> >>> 95670512 > 88225395
> True
> 
> While MD5 is probably faster for some kind of large-data
> cycles-per-byte, due to its 64-byte internal state, SipHash -- the
> "Sip" part standing "Short Input PRF" -- is fast for shorter inputs.
> In practice with the functions we're talking about replacing, there's
> no need to hash 64-bytes. So, SipHash comes out faster and more
> secure.
> 
> I also haven't begun to look focusedly at the assembly my SipHash
> implemention is generating, which means there's still window for even
> more performance improvements.
> 
> Jason
> 
> 
> [1] 
> https://git.zx2c4.com/linux-dev/tree/net/core/secure_seq.c?h=siphash-bench#n194

Now I am quite confused.

George said :

> Cycles per byte on 1024 bytes of data:
>   Pentium Core 2  Ivy
>   4   Duo Bridge
> SipHash-2-4   38.9 8.3 5.8
> HalfSipHash-2-4   12.7 4.5 3.2
> MD58.3 5.7 4.7


That really was for 1024 bytes blocks, so pretty much useless for our
discussion ?

Reading your numbers last week, I thought SipHash was faster, but George
numbers are giving the opposite impression.

I do not have a P4 to make tests, so I only can trust you or George.

Thanks.


--
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: HalfSipHash Acceptable Usage

2016-12-20 Thread Eric Dumazet 
On Tue, 2016-12-20 at 22:28 -0500, George Spelvin wrote:
> > I do not see why SipHash, if faster than MD5 and more secure, would be a
> > problem.
> 
> Because on 32-bit x86, it's slower.
> 
> Cycles per byte on 1024 bytes of data:
>   Pentium Core 2  Ivy
>   4   Duo Bridge
> SipHash-2-4   38.9 8.3 5.8
> HalfSipHash-2-4   12.7 4.5 3.2
> MD58.3 5.7 4.7

So definitely not faster.

38 cycles per byte is a problem, considering IPV6 is ramping up.

But TCP session establishment on P4 is probably not a big deal.
Nobody would expect a P4 to handle gazillions of TCP flows (using a
32bit kernel)

What about SHA performance (syncookies) on P4 ?

Synfloods are probably the only case we might take care of for 2000-era
cpus.





--
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: HalfSipHash Acceptable Usage

2016-12-20 Thread Eric Dumazet 
On Tue, 2016-12-20 at 16:36 -0500, Theodore Ts'o wrote:
> On Mon, Dec 19, 2016 at 06:32:44PM +0100, Jason A. Donenfeld wrote:
> > 1) Anything that requires actual long-term security will use
> > SipHash2-4, with the 64-bit output and the 128-bit key. This includes
> > things like TCP sequence numbers. This seems pretty uncontroversial to
> > me. Seem okay to you?
> 
> Um, why do TCP sequence numbers need long-term security?  So long as
> you rekey every 5 minutes or so, TCP sequence numbers don't need any
> more security than that, since even if you break the key used to
> generate initial sequence numbers seven a minute or two later, any
> pending TCP connections will have timed out long before.
> 
> See the security analysis done in RFC 6528[1], where among other
> things, it points out why MD5 is acceptable with periodic rekeying,
> although there is the concern that this could break certain hueristics
> used when establishing new connections during the TIME-WAIT state.
> 
> [1] https://tools.ietf.org/html/rfc6528


We do not use rekeying for TCP ISN, not anymore after commit
6e5714eaf77d79ae1 (where we switched from MD4 to MD5 )

It might hurt some common cases and I do not believe it is mandated by a
current (ie not obsolete) RFC.

Our clock has a 64 ns resolution and 274 second period (commit
9b42c336d0641) (compared to 4 usec one in RFC 6528)

I do not see why SipHash, if faster than MD5 and more secure, would be a
problem.

Same for syncookies.

BTW, we probably should add a ratelimit on SYNACK retransmits,
because it seems that attackers understood linux kernels resist to
synfloods, and they (the bad guys) use reflection attacks.



--
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] net: fix uninitialized variable issue

2015-12-15 Thread Eric Dumazet 
On Tue, 2015-12-15 at 10:46 -0800, Tadeusz Struk wrote:
> msg_iocb needs to be initialized on the recv/recvfrom path.
> Otherwise afalg will wrongly interpret it as an async call.
> 
> Cc: sta...@vger.kernel.org
> Reported-by: Harald Freudenberger 
> Signed-off-by: Tadeusz Struk 
> ---
>  net/socket.c |1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/socket.c b/net/socket.c
> index dd2c247..80ca820 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1702,6 +1702,7 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, 
> size_t, size,
>   msg.msg_name = addr ? (struct sockaddr *) : NULL;
>   /* We assume all kernel code knows the size of sockaddr_storage */
>   msg.msg_namelen = 0;
> + msg.msg_iocb = NULL;
>   if (sock->file->f_flags & O_NONBLOCK)
>   flags |= MSG_DONTWAIT;
>   err = sock_recvmsg(sock, , iov_iter_count(_iter), flags);
> 

Do not add "Cc: sta...@vger.kernel.org" for networking patches, thanks 
( Documentation/networking/netdev-FAQ.txt )

Do you know when was this bug added ?

Please add the 

Fixes:  12-digit-sha1 ("patch title")

Thanks.


--
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] net: fix uninitialized variable issue

2015-12-15 Thread Eric Dumazet 
On Tue, 2015-12-15 at 14:42 -0800, Tadeusz Struk wrote:

> It was added in this commit: 0345f93138b2 ("net: socket: add support for 
> async operations")
> 

Thanks !


--
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: ipsec impact on performance

2015-12-03 Thread Eric Dumazet 
On Thu, 2015-12-03 at 14:33 -0500, David Miller wrote:
> From: Sowmini Varadhan 
> Date: Tue, 1 Dec 2015 12:59:53 -0500
> 
> > I instrumented iperf with and without ipsec, just using esp-null, 
> > and 1 thread, to keep things simple. I'm seeing some pretty dismal 
> > performance numbers with ipsec, and trying to think of ways to
> > improve this. Here are my findings, please share feedback.
> 
> Doesn't skb_cow_data() contribute significantly to the ESP base cost,
> especially for TCP packets?
> 
> I mean, we're copying every TCP data frame.
> 
> If this is the case, even with GSO/whatever offloads, I expect that
> performance will be roughly halfed.

This reminds me this thing I noticed is that we (un)clone all xmit GRE
GSO packets because of following code in iptunnel_handle_offloads() :

if (skb_is_gso(skb)) {
err = skb_unclone(skb, GFP_ATOMIC);
if (unlikely(err))
goto error;
skb_shinfo(skb)->gso_type |= gso_type_mask;
return skb;
}

This is certainly something we should avoid, since we have ~1500 bytes
of payload in skb->head per TCP skb

Ideally, part of gso_type should belong to skb, not skb_shinfo(skb) :(



--
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: ipsec impact on performance

2015-12-02 Thread Eric Dumazet 
On Wed, 2015-12-02 at 16:12 -0500, Sowmini Varadhan wrote:

> IPv6 would be an interesting academic exercise

Really, you made my day !


--
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: lzo: use kvfree() helper

2014-06-24 Thread Eric Dumazet 
From: Eric Dumazet eduma...@google.com

kvfree() helper is now available, use it instead of open code it.

Signed-off-by: Eric Dumazet eduma...@google.com
---
 crypto/lzo.c |5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/crypto/lzo.c b/crypto/lzo.c
index 252e791d..a8ff2f7 100644
--- a/crypto/lzo.c
+++ b/crypto/lzo.c
@@ -45,10 +45,7 @@ static void lzo_exit(struct crypto_tfm *tfm)
 {
struct lzo_ctx *ctx = crypto_tfm_ctx(tfm);
 
-   if (is_vmalloc_addr(ctx-lzo_comp_mem))
-   vfree(ctx-lzo_comp_mem);
-   else
-   kfree(ctx-lzo_comp_mem);
+   kvfree(ctx-lzo_comp_mem);
 }
 
 static int lzo_compress(struct crypto_tfm *tfm, const u8 *src,


--
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: lzo: try kmalloc() before vmalloc()

2014-05-28 Thread Eric Dumazet 
On Wed, 2014-05-28 at 01:30 +0800, Herbert Xu wrote:

 Looks good to me.  This would seem like something that could be
 generalised to other vmalloc users, no?

BTW, any objections if I send patches to add NUMA allocations into
crypto layer, or was it already considered ?



--
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: lzo: try kmalloc() before vmalloc()

2014-05-27 Thread Eric Dumazet 
From: Eric Dumazet eduma...@google.com

zswap allocates one LZO context per online cpu.

Using vmalloc() for small (16KB) memory areas has drawback of slowing
down /proc/vmallocinfo and /proc/meminfo reads, TLB pressure and poor
NUMA locality, as default NUMA policy at boot time is to interleave
pages :

edumazet:~# grep lzo /proc/vmallocinfo | head -4
0xc90006062000-0xc90006067000   20480 lzo_init+0x1b/0x30 pages=4 
vmalloc N0=2 N1=2
0xc90006067000-0xc9000606c000   20480 lzo_init+0x1b/0x30 pages=4 
vmalloc N0=2 N1=2
0xc9000606c000-0xc90006071000   20480 lzo_init+0x1b/0x30 pages=4 
vmalloc N0=2 N1=2
0xc90006071000-0xc90006076000   20480 lzo_init+0x1b/0x30 pages=4 
vmalloc N0=2 N1=2

This patch tries a regular kmalloc() and fallback to vmalloc in case
memory is too fragmented.

Signed-off-by: Eric Dumazet eduma...@google.com
---
 crypto/lzo.c |   11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/crypto/lzo.c b/crypto/lzo.c
index 1c2aa69c54b8..252e791d0ccc 100644
--- a/crypto/lzo.c
+++ b/crypto/lzo.c
@@ -20,6 +20,7 @@
 #include linux/module.h
 #include linux/crypto.h
 #include linux/vmalloc.h
+#include linux/mm.h
 #include linux/lzo.h
 
 struct lzo_ctx {
@@ -30,7 +31,10 @@ static int lzo_init(struct crypto_tfm *tfm)
 {
struct lzo_ctx *ctx = crypto_tfm_ctx(tfm);
 
-   ctx-lzo_comp_mem = vmalloc(LZO1X_MEM_COMPRESS);
+   ctx-lzo_comp_mem = kmalloc(LZO1X_MEM_COMPRESS,
+   GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT);
+   if (!ctx-lzo_comp_mem)
+   ctx-lzo_comp_mem = vmalloc(LZO1X_MEM_COMPRESS);
if (!ctx-lzo_comp_mem)
return -ENOMEM;
 
@@ -41,7 +45,10 @@ static void lzo_exit(struct crypto_tfm *tfm)
 {
struct lzo_ctx *ctx = crypto_tfm_ctx(tfm);
 
-   vfree(ctx-lzo_comp_mem);
+   if (is_vmalloc_addr(ctx-lzo_comp_mem))
+   vfree(ctx-lzo_comp_mem);
+   else
+   kfree(ctx-lzo_comp_mem);
 }
 
 static int lzo_compress(struct crypto_tfm *tfm, const u8 *src,


--
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: lzo: try kmalloc() before vmalloc()

2014-05-27 Thread Eric Dumazet 
On Wed, 2014-05-28 at 01:30 +0800, Herbert Xu wrote:

 Looks good to me.  This would seem like something that could be
 generalised to other vmalloc users, no?
 

Yep, we have some spots where this is done, and we had attempts in the
past to get a generic allocator / deallocator but this went nowhere,
after a lot of time trying to get this right.

I guess we need to reach some threshold to convince Andrew Morton ;)

$ git grep -n is_vmalloc_addr | wc -l
85



--
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] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST

2013-11-25 Thread Eric Dumazet 
On Mon, 2013-11-25 at 08:42 +0100, Richard Weinberger wrote:

 In the commit message of your patch you wrote For all sendpage() providers, 
 its a transparent change.. Why does AF_ALG need special handling?
 If users have to care about MSG_SENDPAGE_NOTLAST it is no longer really an 
 internal flag.

MSG_SENDPAGE_NOTLAST is an internal (kernel) flag.

Fact that I missed some MSG_MORE 'users' in the kernel was an oversight.

I am not saying your patch is not needed, I am only saying it reverts
a useful TCP optimization, and we can do better, dont we ?



--
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 02/11] inet_diag: pass inet_diag module to netlink_dump_start

2012-10-03 Thread Eric Dumazet 
On Thu, 2012-10-04 at 12:41 +0800, Gao feng wrote:
 set netlink_dump_control.module to avoid panic.
 
 Signed-off-by: Gao feng gaof...@cn.fujitsu.com
 ---
  net/ipv4/inet_diag.c |2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)
 
 diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
 index 535584c..5ffd7bc 100644
 --- a/net/ipv4/inet_diag.c
 +++ b/net/ipv4/inet_diag.c
 @@ -981,6 +981,7 @@ static int inet_diag_rcv_msg_compat(struct sk_buff *skb, 
 struct nlmsghdr *nlh)
   {
   struct netlink_dump_control c = {
   .dump = inet_diag_dump_compat,
 + .module = THIS_MODULE,
   };
   return netlink_dump_start(net-diag_nlsk, skb, nlh, c);
   }
 @@ -1010,6 +1011,7 @@ static int inet_diag_handler_dump(struct sk_buff *skb, 
 struct nlmsghdr *h)
   {
   struct netlink_dump_control c = {
   .dump = inet_diag_dump,
 + .module = THIS_MODULE,
   };
   return netlink_dump_start(net-diag_nlsk, skb, h, c);
   }


I believe Pablo suggestion was to make netlink_dump_start()
automatically pass THIS_MODULE so that we dont need to change all call
sites ?

extern int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
  const struct nlmsghdr *nlh,
  struct netlink_dump_control *control);

static inline int netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
const struct nlmsghdr *nlh,
struct netlink_dump_control *control)
{
control-module = THIS_MODULE;
return __netlink_dump_start(ssk, skb, nlh, control);
}

or :

extern int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
  const struct nlmsghdr *nlh,
  struct netlink_dump_control *control,
  struct module *module);

static inline int netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
const struct nlmsghdr *nlh,
struct netlink_dump_control *control)
{
return __netlink_dump_start(ssk, skb, nlh, control, THIS_MODULE);
}


--
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 01/11] netlink: add reference of module in netlink_dump_start

2012-09-26 Thread Eric Dumazet 
On Wed, 2012-09-26 at 12:52 +0800, Gao feng wrote:

 +int netlink_dump_done(struct netlink_callback *cb)
 +{
 + if (cb-module)
 + module_put(cb-module);
 + return 0;
 +}
 +EXPORT_SYMBOL(netlink_dump_done);
 +

No need to test cb-module being not NULL


int netlink_dump_done(struct netlink_callback *cb)
{
module_put(cb-module);
return 0;
}

Same remark for try_module_get() call


--
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] sha512: reduce stack usage to safe number

2012-01-16 Thread Eric Dumazet 
Le lundi 16 janvier 2012 à 09:56 +, David Laight a écrit :
 Doesn't this badly overflow W[] ..
  
  +#define SHA512_0_15(i, a, b, c, d, e, f, g, h) \
  +   t1 = h + e1(e) + Ch(e, f, g) + sha512_K[i] + W[i];  \
 ...
  +   for (i = 0; i  16; i += 8) {
 ...
  +   SHA512_0_15(i + 7, b, c, d, e, f, g, h, a);
  +   }
 
   David
 
 

No overflow since loop is done for only i=0 and i=8

By the way, I suspect previous code was chosen years ago because this
version uses less stack but adds much more code bloat.


size crypto/sha512_generic.o crypto/sha512_generic_old.o
   textdata bss dec hex filename
  17369 704   0   180734699 crypto/sha512_generic.o
   8249 704   0895322f9 crypto/sha512_generic_old.o



--
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] sha512: make it work, undo percpu message schedule

2012-01-14 Thread Eric Dumazet 
Le samedi 14 janvier 2012 à 21:27 +0300, Alexey Dobriyan a écrit :
 commit f9e2bca6c22d75a289a349f869701214d63b5060
 aka crypto: sha512 - Move message schedule W[80] to static percpu area
 created global message schedule area.


 Signed-off-by: Alexey Dobriyan adobri...@gmail.com
 Cc: sta...@vger.kernel.org
 ---
 
  crypto/sha512_generic.c |6 +-
  1 file changed, 1 insertion(+), 5 deletions(-)
 
 --- a/crypto/sha512_generic.c
 +++ b/crypto/sha512_generic.c
 @@ -21,8 +21,6 @@
  #include linux/percpu.h
  #include asm/byteorder.h
  
 -static DEFINE_PER_CPU(u64[80], msg_schedule);
 -
  static inline u64 Ch(u64 x, u64 y, u64 z)
  {
  return z ^ (x  (y ^ z));
 @@ -89,7 +87,7 @@ sha512_transform(u64 *state, const u8 *input)
   u64 a, b, c, d, e, f, g, h, t1, t2;
  
   int i;
 - u64 *W = get_cpu_var(msg_schedule);
 + u64 W[80];
  
   /* load the input */
  for (i = 0; i  16; i++)
 @@ -128,8 +126,6 @@ sha512_transform(u64 *state, const u8 *input)
  
   /* erase our data */
   a = b = c = d = e = f = g = h = t1 = t2 = 0;
 - memset(W, 0, sizeof(__get_cpu_var(msg_schedule)));
 - put_cpu_var(msg_schedule);
  }
  
  static int

Is it just me or are you ignoring what crypto maintainer and others
thought of your patch ?

You are re-introducing a 640 bytes stack array, how comes it can be
really safe ?

This is too risky, and we provided an alternate patch, not just for fun.


--
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] sha512: make it work, undo percpu message schedule

2012-01-14 Thread Eric Dumazet 
Le samedi 14 janvier 2012 à 13:52 -0800, Linus Torvalds a écrit :
 On Sat, Jan 14, 2012 at 1:46 PM, Eric Dumazet eric.duma...@gmail.com wrote:
 
  This is too risky, and we provided an alternate patch, not just for fun.
 
 Did you see the second patch?
 

I saw it and felt it was not a stable material.

And Herbert sent an alternate patch, then I sent a V3 patch.

 The one that got rid of the *stupid* 80-entry array?
 

Maybe, I didnt wrote this code, and I am very glad this code can be
smarter and all.



--
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: sha512: make it work, undo percpu message schedule

2012-01-13 Thread Eric Dumazet 
Le vendredi 13 janvier 2012 à 18:08 +1100, Herbert Xu a écrit :
 On Fri, Jan 13, 2012 at 02:55:14AM +0300, Alexey Dobriyan wrote:
 
  Herbert, I couldn't come up with a single scenario. :-(
  But the bug is easy to reproduce.
 
 OK, does this patch work for you?
 
 commit 31f4e55c09c1170f8b813c14b1299b70f50db414
 Author: Herbert Xu herb...@gondor.apana.org.au
 Date:   Fri Jan 13 18:06:50 2012 +1100
 
 crypto: sha512 - Fix msg_schedule race
 
 The percpu msg_schedule setup was unsafe as a user in a process
 context can be interrupted by a softirq user which would then
 scribble over the exact same work area.  This was discovered by
 Steffen Klassert.
 
 This patch based on ideas from Eric Dumazet fixes this by using
 two independent work areas.
 
 Reported-by: Alexey Dobriyan adobri...@gmail.com
 Signed-off-by: Herbert Xu herb...@gondor.apana.org.au
 

I wonder ...

With 4096 cpus, do we really want to reserve 5242880 bytes of memory for
this function ?

What about following patch instead ?

(Trying a dynamic memory allocation, and fallback on a single
pre-allocated bloc of memory, shared by all cpus, protected by a
spinlock)

diff --git a/crypto/sha512_generic.c b/crypto/sha512_generic.c
index 9ed9f60..5c80a76 100644
--- a/crypto/sha512_generic.c
+++ b/crypto/sha512_generic.c
@@ -21,7 +21,6 @@
 #include linux/percpu.h
 #include asm/byteorder.h
 
-static DEFINE_PER_CPU(u64[80], msg_schedule);
 
 static inline u64 Ch(u64 x, u64 y, u64 z)
 {
@@ -87,10 +86,16 @@ static void
 sha512_transform(u64 *state, const u8 *input)
 {
u64 a, b, c, d, e, f, g, h, t1, t2;
-
+   static u64 msg_schedule[80];
+   static DEFINE_SPINLOCK(msg_schedule_lock);
int i;
-   u64 *W = get_cpu_var(msg_schedule);
+   u64 *W = kzalloc(sizeof(msg_schedule), GFP_ATOMIC | __GFP_NOWARN);
 
+   
+   if (!W) {
+   spin_lock_bh(msg_schedule_lock);
+   W = msg_schedule;
+   }
/* load the input */
 for (i = 0; i  16; i++)
 LOAD_OP(i, W, input);
@@ -128,8 +133,11 @@ sha512_transform(u64 *state, const u8 *input)
 
/* erase our data */
a = b = c = d = e = f = g = h = t1 = t2 = 0;
-   memset(W, 0, sizeof(__get_cpu_var(msg_schedule)));
-   put_cpu_var(msg_schedule);
+   memset(W, 0, sizeof(msg_schedule));
+   if (W == msg_schedule)
+   spin_unlock_bh(msg_schedule_lock);
+   else
+   kfree(W);
 }
 
 static int


--
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: sha512: make it work, undo percpu message schedule

2012-01-13 Thread Eric Dumazet 
Le vendredi 13 janvier 2012 à 11:35 +0100, Eric Dumazet a écrit :

 I wonder ...
 
 With 4096 cpus, do we really want to reserve 5242880 bytes of memory for
 this function ?
 
 What about following patch instead ?
 
 (Trying a dynamic memory allocation, and fallback on a single
 pre-allocated bloc of memory, shared by all cpus, protected by a
 spinlock)
 
 diff --git a/crypto/sha512_generic.c b/crypto/sha512_generic.c
 index 9ed9f60..5c80a76 100644
 --- a/crypto/sha512_generic.c
 +++ b/crypto/sha512_generic.c
 @@ -21,7 +21,6 @@
  #include linux/percpu.h
  #include asm/byteorder.h
  
 -static DEFINE_PER_CPU(u64[80], msg_schedule);
  
  static inline u64 Ch(u64 x, u64 y, u64 z)
  {
 @@ -87,10 +86,16 @@ static void
  sha512_transform(u64 *state, const u8 *input)
  {
   u64 a, b, c, d, e, f, g, h, t1, t2;
 -
 + static u64 msg_schedule[80];
 + static DEFINE_SPINLOCK(msg_schedule_lock);
   int i;
 - u64 *W = get_cpu_var(msg_schedule);
 + u64 *W = kzalloc(sizeof(msg_schedule), GFP_ATOMIC | __GFP_NOWARN);
  

And a plain kmalloc() is enough, since we fully initialize the array a
bit later.

for (i = 0; i  16; i++)
LOAD_OP(i, W, input);
for (i = 16; i  80; i++) {
BLEND_OP(i, W);
}




--
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: sha512: make it work, undo percpu message schedule

2012-01-13 Thread Eric Dumazet 
Le vendredi 13 janvier 2012 à 11:41 +0100, Eric Dumazet a écrit :

 And a plain kmalloc() is enough, since we fully initialize the array a
 bit later.
 
   for (i = 0; i  16; i++)
   LOAD_OP(i, W, input);
   for (i = 16; i  80; i++) {
   BLEND_OP(i, W);
   }


Here is an official patch ;)

[PATCH v3] crypto: sha512 - Fix msg_schedule race

The percpu msg_schedule setup was unsafe as a user in a process
context can be interrupted by a softirq user which would then
scribble over the exact same work area.

Bug reported by Alexey Dobriyan, and diagnosed by Steffen Klassert.

Bug added in commit f9e2bca6c22 (crypto: sha512 - Move message schedule
W[80] to static percpu area)

Try a dynamic memory allocation, and fallback using a single static
area, guarded by a spinlock.

This removes use of percpu memory.

Reported-by: Alexey Dobriyan adobri...@gmail.com
Signed-off-by: Eric Dumazet eric.duma...@gmail.com
CC: Herbert Xu herb...@gondor.apana.org.au
CC: Adrian-Ken Rueegsegger k...@codelabs.ch
CC: Steffen Klassert steffen.klass...@secunet.com
---
 crypto/sha512_generic.c |   18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/crypto/sha512_generic.c b/crypto/sha512_generic.c
index 9ed9f60..b52ef9b 100644
--- a/crypto/sha512_generic.c
+++ b/crypto/sha512_generic.c
@@ -18,10 +18,8 @@
 #include linux/crypto.h
 #include linux/types.h
 #include crypto/sha.h
-#include linux/percpu.h
 #include asm/byteorder.h
 
-static DEFINE_PER_CPU(u64[80], msg_schedule);
 
 static inline u64 Ch(u64 x, u64 y, u64 z)
 {
@@ -87,10 +85,15 @@ static void
 sha512_transform(u64 *state, const u8 *input)
 {
u64 a, b, c, d, e, f, g, h, t1, t2;
-
+   static u64 msg_schedule[80];
+   static DEFINE_SPINLOCK(msg_schedule_lock);
int i;
-   u64 *W = get_cpu_var(msg_schedule);
+   u64 *W = kmalloc(sizeof(msg_schedule), GFP_ATOMIC | __GFP_NOWARN);
 
+   if (!W) {
+   spin_lock_bh(msg_schedule_lock);
+   W = msg_schedule;
+   }
/* load the input */
 for (i = 0; i  16; i++)
 LOAD_OP(i, W, input);
@@ -128,8 +131,11 @@ sha512_transform(u64 *state, const u8 *input)
 
/* erase our data */
a = b = c = d = e = f = g = h = t1 = t2 = 0;
-   memset(W, 0, sizeof(__get_cpu_var(msg_schedule)));
-   put_cpu_var(msg_schedule);
+   memset(W, 0, sizeof(msg_schedule));
+   if (W == msg_schedule)
+   spin_unlock_bh(msg_schedule_lock);
+   else
+   kfree(W);
 }
 
 static int


--
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: sha512: make it work, undo percpu message schedule

2012-01-13 Thread Eric Dumazet 
Le vendredi 13 janvier 2012 à 13:33 +0200, Alexey Dobriyan a écrit :
 On 1/13/12, Eric Dumazet eric.duma...@gmail.com wrote:
 
  +   static u64 msg_schedule[80];
  +   static DEFINE_SPINLOCK(msg_schedule_lock);
 
 No guys, no.
 
 SHA-512 only needs u64[16] running window for message scheduling.
 
 I'm sending whitespace mangled patch which is only tested
 with selfcryptotest passed, so you won't apply something complex.
 
 Stackspace usage drops down to like this:
 
 -139:   48 81 ec c8 02 00 00sub$0x2c8,%rsp
 +136:   48 81 ec 18 01 00 00sub$0x118,%rsp
 
 --- a/crypto/sha512_generic.c
 +++ b/crypto/sha512_generic.c
 @@ -21,8 +21,6 @@
  #include linux/percpu.h
  #include asm/byteorder.h
 
 -static DEFINE_PER_CPU(u64[80], msg_schedule);
 -
  static inline u64 Ch(u64 x, u64 y, u64 z)
  {
  return z ^ (x  (y ^ z));
 @@ -80,7 +78,7 @@ static inline void LOAD_OP(int I, u64 *W, const u8 *input)
 
  static inline void BLEND_OP(int I, u64 *W)
  {
 - W[I] = s1(W[I-2]) + W[I-7] + s0(W[I-15]) + W[I-16];
 + W[I%16] = s1(W[(I-2)%16]) + W[(I-7)%16] + s0(W[(I-15)%16]) + W[I%16];
  }
 
  static void
 @@ -89,38 +87,48 @@ sha512_transform(u64 *state, const u8 *input)
   u64 a, b, c, d, e, f, g, h, t1, t2;
 
   int i;
 - u64 *W = get_cpu_var(msg_schedule);
 + u64 W[16];
 
   /* load the input */
  for (i = 0; i  16; i++)
  LOAD_OP(i, W, input);
 
 -for (i = 16; i  80; i++) {
 -BLEND_OP(i, W);
 -}
 -
   /* load the state into our registers */
   a=state[0];   b=state[1];   c=state[2];   d=state[3];
   e=state[4];   f=state[5];   g=state[6];   h=state[7];
 
 - /* now iterate */
 - for (i=0; i80; i+=8) {
 - t1 = h + e1(e) + Ch(e,f,g) + sha512_K[i  ] + W[i  ];
 - t2 = e0(a) + Maj(a,b,c);d+=t1;h=t1+t2;
 - t1 = g + e1(d) + Ch(d,e,f) + sha512_K[i+1] + W[i+1];
 - t2 = e0(h) + Maj(h,a,b);c+=t1;g=t1+t2;
 - t1 = f + e1(c) + Ch(c,d,e) + sha512_K[i+2] + W[i+2];
 - t2 = e0(g) + Maj(g,h,a);b+=t1;f=t1+t2;
 - t1 = e + e1(b) + Ch(b,c,d) + sha512_K[i+3] + W[i+3];
 - t2 = e0(f) + Maj(f,g,h);a+=t1;e=t1+t2;
 - t1 = d + e1(a) + Ch(a,b,c) + sha512_K[i+4] + W[i+4];
 - t2 = e0(e) + Maj(e,f,g);h+=t1;d=t1+t2;
 - t1 = c + e1(h) + Ch(h,a,b) + sha512_K[i+5] + W[i+5];
 - t2 = e0(d) + Maj(d,e,f);g+=t1;c=t1+t2;
 - t1 = b + e1(g) + Ch(g,h,a) + sha512_K[i+6] + W[i+6];
 - t2 = e0(c) + Maj(c,d,e);f+=t1;b=t1+t2;
 - t1 = a + e1(f) + Ch(f,g,h) + sha512_K[i+7] + W[i+7];
 - t2 = e0(b) + Maj(b,c,d);e+=t1;a=t1+t2;
 +#define SHA512_0_15(i, a, b, c, d, e, f, g, h)   \
 + t1 = h + e1(e) + Ch(e, f, g) + sha512_K[i] + W[i];  \
 + t2 = e0(a) + Maj(a,b,c);\
 + d += t1;\
 + h = t1 + t2
 +
 +#define SHA512_16_79(i, a, b, c, d, e, f, g, h)  \
 + BLEND_OP(i, W); \
 + t1 = h + e1(e) + Ch(e, f, g) + sha512_K[i] + W[(i)%16]; \
 + t2 = e0(a) + Maj(a,b,c);\
 + d += t1;\
 + h = t1 + t2
 +
 + for (i = 0; i  16; i += 8) {
 + SHA512_0_15(i, a, b, c, d, e, f, g, h);
 + SHA512_0_15(i + 1, h, a, b, c, d, e, f, g);
 + SHA512_0_15(i + 2, g, h, a, b, c, d, e, f);
 + SHA512_0_15(i + 3, f, g, h, a, b, c, d, e);
 + SHA512_0_15(i + 4, e, f, g, h, a, b, c, d);
 + SHA512_0_15(i + 5, d, e, f, g, h, a, b, c);
 + SHA512_0_15(i + 6, c, d, e, f, g, h, a, b);
 + SHA512_0_15(i + 7, b, c, d, e, f, g, h, a);
 + }
 + for (i = 16; i  80; i += 8) {
 + SHA512_16_79(i, a, b, c, d, e, f, g, h);
 + SHA512_16_79(i + 1, h, a, b, c, d, e, f, g);
 + SHA512_16_79(i + 2, g, h, a, b, c, d, e, f);
 + SHA512_16_79(i + 3, f, g, h, a, b, c, d, e);
 + SHA512_16_79(i + 4, e, f, g, h, a, b, c, d);
 + SHA512_16_79(i + 5, d, e, f, g, h, a, b, c);
 + SHA512_16_79(i + 6, c, d, e, f, g, h, a, b);
 + SHA512_16_79(i + 7, b, c, d, e, f, g, h, a);
   }
 
   state[0] += a; state[1] += b; state[2] += c; state[3] += d;
 @@ -128,8 +136,6 @@ sha512_transform(u64 *state, const u8 *input)
 
   /* erase our data */
   a = b = c = d = e = f = g = h = t1 = t2 = 0;
 - memset(W, 0, sizeof(__get_cpu_var(msg_schedule)));
 - put_cpu_var(msg_schedule);
  }
 
  static int


Even if its true, its not stable material.

stable teams want obvious patches.



--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord

Re: sha512: make it work, undo percpu message schedule

2012-01-12 Thread Eric Dumazet 
Le vendredi 13 janvier 2012 à 07:22 +0100, Steffen Klassert a écrit :
 On Wed, Jan 11, 2012 at 11:36:11AM +1100, Herbert Xu wrote:
  On Wed, Jan 11, 2012 at 03:00:40AM +0300, Alexey Dobriyan wrote:
   commit f9e2bca6c22d75a289a349f869701214d63b5060
   aka crypto: sha512 - Move message schedule W[80] to static percpu area
   created global message schedule area.
   
   If sha512_update will ever be entered twice, hilarity ensures.
  
  Hmm, do you know why this happens? On the face of it this shouldn't
  be possible as preemption is disabled.
  
 
 I did not try to reproduce, but this looks like a race of the 'local out'
 and the receive packet path. On 'lokal out' bottom halves are enabled,
 so could be interrupted by the NET_RX_SOFTIRQ while doing a sha512_update.
 The NET_RX_SOFTIRQ could invoke sha512_update too, that would corrupt the
 hash value. My guess could be checked easily by disabling the bottom halves
 before the percpu value is fetched.

Good catch. It can be generalized to any interrupts (soft and hard)

Another solution is using two blocks, one used from interrupt context.

static DEFINE_PER_CPU(u64[80], msg_schedule);
static DEFINE_PER_CPU(u64[80], msg_schedule_irq);

(Like we do for SNMP mibs on !x86 arches)



--
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