Mailing List Archive

[PATCH] Fix broken mlock detection
We need to be careful when casting a pointer to a `long int`: the
highest bit might be set, in which case the result is a negative number.

In this instance, it is fatal: we now take the modulus of that negative
number with regards to the page size, and subtract it from the page
size. So what should be a number that is smaller than the page size is
now larger than the page size.

As a consequence, we do not try to lock a 4096-byte block that is at the
page size boundary inside a `malloc()`ed block, but we try to do that
_outside_ the block.

Which means that we are not at all detecting whether `mlock()` is
broken.

This actually happened here, in the i686 MSYS2 build of libgcrypt.

Let's be very careful to case the pointer to an _unsigned_ value
instead.

Note: technically, we should cast the pointer to a `size_t`. But since
we only need the remainder modulo the page size (which is a power of
two) anyway, it does not matter whether we clip, say, a 64-bit `size_t`
to a 32-bit `unsigned long`. It does matter, though, whether we
mistakenly turn the remainder into a negative one.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
acinclude.m4 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 3c8dfba7..4a2a83c0 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -236,7 +236,7 @@ int main()
pool = malloc( 4096 + pgsize );
if( !pool )
return 2;
- pool += (pgsize - ((long int)pool % pgsize));
+ pool += (pgsize - ((unsigned long int)pool % pgsize));

err = mlock( pool, 4096 );
if( !err || errno == EPERM || errno == EAGAIN)
--
2.31.1



_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel
Re: [PATCH] Fix broken mlock detection [ In reply to ]
Hi!

On Wed, 16 Jun 2021 10:07, Johannes Schindelin said:

> Which means that we are not at all detecting whether `mlock()` is
> broken.

Thanks for your correct analysis. I wrote this test code 23 years ago
and this is the first report. Seems that until now this has never been
tried on systems which allocate memory above 2 GiB.

> This actually happened here, in the i686 MSYS2 build of libgcrypt.

Please take care: I would not suggest to build the Windows version with
MSYS - the only supported toolchain for Windows is gcc.

> we only need the remainder modulo the page size (which is a power of
> two) anyway, it does not matter whether we clip, say, a 64-bit `size_t`
> to a 32-bit `unsigned long`. It does matter, though, whether we

Yep. I would anyway use size_t here to avoid questions about the
reasoning. In fact secmem.c uses uintptr_t but that is a bit too
complicated to use in this configure test.

Thanks,

Werner

--
Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz.
Re: [PATCH] Fix broken mlock detection [ In reply to ]
Hi Werner,

On Wed, 16 Jun 2021, Werner Koch wrote:

> On Wed, 16 Jun 2021 10:07, Johannes Schindelin said:
>
> > Which means that we are not at all detecting whether `mlock()` is
> > broken.
>
> Thanks for your correct analysis. I wrote this test code 23 years ago
> and this is the first report. Seems that until now this has never been
> tried on systems which allocate memory above 2 GiB.

Right. 23 years ago, some people still believed that nobody will ever need
more than 640 kilobyte of RAM ;-)

BTW I _suspect_ that the reason I ran into this is a recent change in
Cygwin and/or Windows 10. I cannot recall seeing `malloc()`ed blocks above
0x80000000.

> > This actually happened here, in the i686 MSYS2 build of libgcrypt.
>
> Please take care: I would not suggest to build the Windows version with
> MSYS - the only supported toolchain for Windows is gcc.

Oh, I should have been clearer: I _am_ building using GCC.

MSYS2 is based partially on Cygwin (the MSYS2 runtime is a close fork of
the Cygwin runtime, to provide a POSIX emulation layer), and partially on
ArchLinux (from where it inherits its package management system, pacman).

It is a well-tested system, and it is used as an important block of Git
for Windows: millions of users already rely on it for over five years.

In other words: I am not worried about building and using GNU Privacy
Guard and libgcrypt in MSYS2's context ;-)

> > we only need the remainder modulo the page size (which is a power of
> > two) anyway, it does not matter whether we clip, say, a 64-bit `size_t`
> > to a 32-bit `unsigned long`. It does matter, though, whether we
>
> Yep. I would anyway use size_t here to avoid questions about the
> reasoning. In fact secmem.c uses uintptr_t but that is a bit too
> complicated to use in this configure test.

As long as you can be sure that the type that is used is actually defined,
I do not care which one you use. I used `unsigned long` because there is
no question that it is defined here, whereas I have run into compile
problems in the past on systems where `size_t` was not defined.

Ciao,
Dscho

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel