Обсуждение: 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
Вложения
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=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.) 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