Re: [PATCH v2 2/2] crypto, x86: SSSE3 based SHA1 implementation for x86-64
On Mon, Aug 8, 2011 at 1:48 PM, Locktyukhin, Maxim maxim.locktyuk...@intel.com wrote: 20 (and more) cycles per byte shown below are not reasonable numbers for SHA-1 - ~6 c/b (as can be seen in some of the results for Core2) is the expected results ... Ten years ago, on Pentium II, one benchmark showed 13 cycles/byte for SHA-1. http://www.freeswan.org/freeswan_trees/freeswan-2.06/doc/performance.html#perf.estimate -- 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] lib/sha1: remove memsets and allocate workspace on the stack
On Mon, 2011-08-08 at 16:07 -0700, Mandeep Singh Baines wrote: The previous implementation required the workspace to be passed in as a parameter. This prevents the compiler from being able to store the workspace in registers. I've also removed the memset since that also prevents the compiler from storing the workspace in registers. I did a similar patch locally. There is no loss of security due to removing the memset. It would be a bug for the stack to leak to userspace. However, a defence-in-depth argument could be made for keeping the clearing of the workspace. You should add #include linux/crypthash.h to lib/sha1.c and perhaps rationalize the use of __u8 and char for the second argument to sha_transform in the definition and uses. For defense in depth, a bool could be added to sha_transform like: void sha_transform(__u32 *digest, const char *data, bool wipe); and the internal workspace memset after use if wipe set though perhaps the memset could be a compile time option like CONFIG_CRYPTO_DEFENSE_IN_DEPTH or such instead. diff --git a/drivers/char/random.c b/drivers/char/random.c [] @@ -816,13 +816,13 @@ static size_t account(struct entropy_store *r, size_t nbytes, int min, static void extract_buf(struct entropy_store *r, __u8 *out) [] - sha_transform(hash, (__u8 *)(r-pool + i), workspace); + sha_transform(hash, (__u8 *)(r-pool + i)); [] diff --git a/include/linux/cryptohash.h b/include/linux/cryptohash.h [] @@ -3,10 +3,9 @@ [] -void sha_transform(__u32 *digest, const char *data, __u32 *W); +void sha_transform(__u32 *digest, const char *data); [] diff --git a/lib/sha1.c b/lib/sha1.c [] +void sha_transform(__u32 *digest, const char *data) [] diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c [] @@ -50,7 +49,7 @@ static u32 cookie_hash(__be32 saddr, __be32 daddr, __be16 sport, __be16 dport, [] - sha_transform(tmp + 16, (__u8 *)tmp, tmp + 16 + 5); + sha_transform(tmp + 16, (__u8 *)tmp); [] diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c [] @@ -2511,8 +2510,7 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst, [] sha_transform((__u32 *)xvp-cookie_bakery[0], - (char *)mess, - workspace[0]); + (char *)mess); [] diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c [] @@ -81,7 +80,7 @@ static u32 cookie_hash(const struct in6_addr *saddr, const struct in6_addr *dadd [] - sha_transform(tmp + 16, (__u8 *)tmp, tmp + 16 + 5); + sha_transform(tmp + 16, (__u8 *)tmp); -- 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] lib/sha1: remove memsets and allocate workspace on the stack
On Mon, Aug 8, 2011 at 4:07 PM, Mandeep Singh Baines m...@chromium.org wrote: There is no loss of security due to removing the memset. It would be a bug for the stack to leak to userspace. However, a defence-in-depth argument could be made for keeping the clearing of the workspace. So I'm nervous about this just because I can see the security crazies rising up about this. The fact is, in our current code in drivers/char/random.c, we do have a memset() of the workspace buffer on the stack, and any competent compiler should actually just remove it, because it's dead memory (and the compiler can *see* that it's dead memory). Of course, I don't know if gcc does notice that, but it's a prime example of code that looks secure, but has absolutely zero actual real security. Getting rid of the memset() is actually better for *real* security, in that at least it's not some kind of pointless security theater. But I can see some people wanting to add a memory barrier or something to force the memset() to actually take place. So I dunno. Arguably it's theoretically possible to find random data on the stack, and maybe it can even be interesting (although I don't think the last 64 bytes of SHA1 state is all that exciting myself). Personally, I consider it unlikely as hell to be relevant to anybody, and anybody who has access to the kernel stack has *much* more direct security holes than some random data that they can use. But the patch still makes me worry about the brouhaha from some people. Linus -- 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] lib/sha1: remove memsets and allocate workspace on the stack
Joe Perches (j...@perches.com) wrote: On Mon, 2011-08-08 at 16:07 -0700, Mandeep Singh Baines wrote: The previous implementation required the workspace to be passed in as a parameter. This prevents the compiler from being able to store the workspace in registers. I've also removed the memset since that also prevents the compiler from storing the workspace in registers. I did a similar patch locally. There is no loss of security due to removing the memset. It would be a bug for the stack to leak to userspace. However, a defence-in-depth argument could be made for keeping the clearing of the workspace. You should add #include linux/crypthash.h to lib/sha1.c and perhaps rationalize the use of __u8 and char for the second argument to sha_transform in the definition and uses. For defense in depth, a bool could be added to sha_transform like: void sha_transform(__u32 *digest, const char *data, bool wipe); This seems reasonable to me. In our case, there is no secrecy issue. We hash the blocks of the RO root filesystem. There is nothing secret about the rootfs blocks or their hash. We use the hashes to verify that the blocks haven't been modified by an attacker. We don't call sha_tranform directly. We use crypto_hash_digest. So maybe add a wipe param there. I'm happy to work on or test such a patch if folks think its interesting. Its saves me 190 ms on a 6 second boot. I suspect there may be other hash intense applications that also don't need secracy. and the internal workspace memset after use if wipe set though perhaps the memset could be a compile time option like CONFIG_CRYPTO_DEFENSE_IN_DEPTH or such instead. Maybe instead use wipe=0 in code paths where you don't need secracy. diff --git a/drivers/char/random.c b/drivers/char/random.c [] @@ -816,13 +816,13 @@ static size_t account(struct entropy_store *r, size_t nbytes, int min, static void extract_buf(struct entropy_store *r, __u8 *out) [] - sha_transform(hash, (__u8 *)(r-pool + i), workspace); + sha_transform(hash, (__u8 *)(r-pool + i)); [] diff --git a/include/linux/cryptohash.h b/include/linux/cryptohash.h [] @@ -3,10 +3,9 @@ [] -void sha_transform(__u32 *digest, const char *data, __u32 *W); +void sha_transform(__u32 *digest, const char *data); [] diff --git a/lib/sha1.c b/lib/sha1.c [] +void sha_transform(__u32 *digest, const char *data) [] diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c [] @@ -50,7 +49,7 @@ static u32 cookie_hash(__be32 saddr, __be32 daddr, __be16 sport, __be16 dport, [] - sha_transform(tmp + 16, (__u8 *)tmp, tmp + 16 + 5); + sha_transform(tmp + 16, (__u8 *)tmp); [] diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c [] @@ -2511,8 +2510,7 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst, [] sha_transform((__u32 *)xvp-cookie_bakery[0], - (char *)mess, - workspace[0]); + (char *)mess); [] diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c [] @@ -81,7 +80,7 @@ static u32 cookie_hash(const struct in6_addr *saddr, const struct in6_addr *dadd [] - sha_transform(tmp + 16, (__u8 *)tmp, tmp + 16 + 5); + sha_transform(tmp + 16, (__u8 *)tmp); -- 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
