Dead stores in src/common/sha2.c
I ran clang checker and noticed these. It looks like the
sha2 implementation is trying to zero out state on exit, but
clang checker finds at least 'a' is a dead store.
Should we fix this?
Is something like the attached sensible?
Is there a common/better approach to zero-out in PG ?
Garick
Attachments:
pg-sha2-dead-store.patchtext/plain; name=pg-sha2-dead-store.patchDownload
diff --git a/src/common/sha2.c b/src/common/sha2.c
index ae0a1a3..9c19ef2 100644
--- a/src/common/sha2.c
+++ b/src/common/sha2.c
@@ -457,7 +457,16 @@ SHA256_Transform(pg_sha256_ctx *context, const uint8 *data)
context->state[7] += h;
/* Clean up */
- a = b = c = d = e = f = g = h = T1 = T2 = 0;
+ *((volatile uint32 *) &a) = 0;
+ *((volatile uint32 *) &b) = 0;
+ *((volatile uint32 *) &c) = 0;
+ *((volatile uint32 *) &d) = 0;
+ *((volatile uint32 *) &e) = 0;
+ *((volatile uint32 *) &f) = 0;
+ *((volatile uint32 *) &g) = 0;
+ *((volatile uint32 *) &h) = 0;
+ *((volatile uint32 *) &T1) = 0;
+ *((volatile uint32 *) &T2) = 0;
}
#endif /* SHA2_UNROLL_TRANSFORM */
@@ -693,7 +702,15 @@ SHA512_Transform(pg_sha512_ctx *context, const uint8 *data)
context->state[7] += h;
/* Clean up */
- a = b = c = d = e = f = g = h = T1 = 0;
+ *((volatile uint32 *) &a) = 0;
+ *((volatile uint32 *) &b) = 0;
+ *((volatile uint32 *) &c) = 0;
+ *((volatile uint32 *) &d) = 0;
+ *((volatile uint32 *) &e) = 0;
+ *((volatile uint32 *) &f) = 0;
+ *((volatile uint32 *) &g) = 0;
+ *((volatile uint32 *) &h) = 0;
+ *((volatile uint32 *) &T1) = 0;
}
#else /* SHA2_UNROLL_TRANSFORM */
@@ -783,7 +800,16 @@ SHA512_Transform(pg_sha512_ctx *context, const uint8 *data)
context->state[7] += h;
/* Clean up */
- a = b = c = d = e = f = g = h = T1 = T2 = 0;
+ *((volatile uint32 *) &a) = 0;
+ *((volatile uint32 *) &b) = 0;
+ *((volatile uint32 *) &c) = 0;
+ *((volatile uint32 *) &d) = 0;
+ *((volatile uint32 *) &e) = 0;
+ *((volatile uint32 *) &f) = 0;
+ *((volatile uint32 *) &g) = 0;
+ *((volatile uint32 *) &h) = 0;
+ *((volatile uint32 *) &T1) = 0;
+ *((volatile uint32 *) &T2) = 0;
}
#endif /* SHA2_UNROLL_TRANSFORM */
On Wed, May 29, 2019 at 01:24:19PM +0000, Hamlin, Garick L wrote:
I ran clang checker and noticed these. It looks like the
sha2 implementation is trying to zero out state on exit, but
clang checker finds at least 'a' is a dead store.Should we fix this?
Is something like the attached sensible?
Is there a common/better approach to zero-out in PG ?
This code comes from the SHA-2 implementation of OpenBSD, so it is not
adapted to directly touch it. What's the current state of this code
in upstream? Should we perhaps try to sync with the upstream
implementation instead?
After a quick search I am not seeing that this area has actually
changed:
http://fxr.watson.org/fxr/source/crypto/sha2.c?v=OPENBSD
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Wed, May 29, 2019 at 01:24:19PM +0000, Hamlin, Garick L wrote:
I ran clang checker and noticed these. It looks like the
sha2 implementation is trying to zero out state on exit, but
clang checker finds at least 'a' is a dead store.Should we fix this?
Is something like the attached sensible?
Is there a common/better approach to zero-out in PG ?
This code comes from the SHA-2 implementation of OpenBSD, so it is not
adapted to directly touch it. What's the current state of this code
in upstream? Should we perhaps try to sync with the upstream
implementation instead?
After a quick search I am not seeing that this area has actually
changed:
http://fxr.watson.org/fxr/source/crypto/sha2.c?v=OPENBSD
Hm ... plastering "volatile"s all over it isn't good for readability
or for quality of the generated code. (In particular, I'm worried
about this patch causing all those variables to be forced into memory
instead of registers.)
At the same time, I'm not sure if we should just write this off as an
ignorable warning. If the C compiler concludes these are dead stores
it'll probably optimize them away, leading to not accomplishing the
goal of wiping the values.
On the third hand, that goal may not be worth much, particularly not
if the variables do get kept in registers.
regards, tom lane
On Wed, May 29, 2019 at 11:01:05AM -0400, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
On Wed, May 29, 2019 at 01:24:19PM +0000, Hamlin, Garick L wrote:
I ran clang checker and noticed these. It looks like the
sha2 implementation is trying to zero out state on exit, but
clang checker finds at least 'a' is a dead store.Should we fix this?
Is something like the attached sensible?
Is there a common/better approach to zero-out in PG ?This code comes from the SHA-2 implementation of OpenBSD, so it is not
adapted to directly touch it. What's the current state of this code
in upstream? Should we perhaps try to sync with the upstream
implementation instead?
After a quick search I am not seeing that this area has actually
changed:
http://fxr.watson.org/fxr/source/crypto/sha2.c?v=OPENBSDHm ... plastering "volatile"s all over it isn't good for readability
or for quality of the generated code. (In particular, I'm worried
about this patch causing all those variables to be forced into memory
instead of registers.)
Yeah, I don't actually think it's a great approach which is why I
was wondering what if PG had a right approach. I figured it was
the clearest way to start the discussion. Especially, since I wasn't
sure if people would want to fix it.
At the same time, I'm not sure if we should just write this off as an
ignorable warning. If the C compiler concludes these are dead stores
it'll probably optimize them away, leading to not accomplishing the
goal of wiping the values.
Yeah, I mean it's odd to put code in to zero/hide state knowing it's
probably optimized out.
We could also take it out, but maybe it does help somewhere?
... or put in a comment that says: This probably gets optimized away, but
we don't consider it much of a risk.
On the third hand, that goal may not be worth much, particularly not
if the variables do get kept in registers.
I haven't looked at the asm.
Maybe they are in registers...
Garick
On 29/05/2019 18:47, Hamlin, Garick L wrote:
On Wed, May 29, 2019 at 11:01:05AM -0400, Tom Lane wrote:
At the same time, I'm not sure if we should just write this off as an
ignorable warning. If the C compiler concludes these are dead stores
it'll probably optimize them away, leading to not accomplishing the
goal of wiping the values.Yeah, I mean it's odd to put code in to zero/hide state knowing it's
probably optimized out.We could also take it out, but maybe it does help somewhere?
... or put in a comment that says: This probably gets optimized away, but
we don't consider it much of a risk.
There's a function called explicit_bzero() in glibc, for this purpose.
See
https://www.gnu.org/software/libc/manual/html_node/Erasing-Sensitive-Data.html.
It's not totally portable, but it's also available in some BSDs, at
least. That documentation mentions the possibility that it might force
variables to be stored in memory that would've otherwise been kept only
in registers, but says that in most situations it's nevertheless better
to use explicit_bero() than not. So I guess we should use that, if it's
available.
- Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 29/05/2019 18:47, Hamlin, Garick L wrote:
On Wed, May 29, 2019 at 11:01:05AM -0400, Tom Lane wrote:
At the same time, I'm not sure if we should just write this off as an
ignorable warning. If the C compiler concludes these are dead stores
it'll probably optimize them away, leading to not accomplishing the
goal of wiping the values.
Yeah, I mean it's odd to put code in to zero/hide state knowing it's
probably optimized out.
We could also take it out, but maybe it does help somewhere?
... or put in a comment that says: This probably gets optimized away, but
we don't consider it much of a risk.
There's a function called explicit_bzero() in glibc, for this purpose.
See
https://www.gnu.org/software/libc/manual/html_node/Erasing-Sensitive-Data.html.
It's not totally portable, but it's also available in some BSDs, at
least. That documentation mentions the possibility that it might force
variables to be stored in memory that would've otherwise been kept only
in registers, but says that in most situations it's nevertheless better
to use explicit_bero() than not. So I guess we should use that, if it's
available.
Meh. After looking at this closer, I'm convinced that doing anything
that might force the variables into memory would be utterly stupid.
Aside from any performance penalty we'd take, that would make their
values more observable not less so.
In any case, though, it's very hard to see why we should care in the
least. Somebody who can observe the contents of server memory (or
application memory, in the case of frontend usage) can surely extract
the input or output of the SHA2 transform, which is going to be way
more useful than a few intermediate values.
So I think we should either do nothing, or suppress this warning by
commenting out the variable-zeroing.
(Note that an eyeball scan finds several more dead zeroings besides
the ones in Garick's patch. Some of them are ifdef'd out ...)
regards, tom lane