mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] Bug in mmap_fixed()
@ 2020-09-04 19:52 Markus Wichmann
  2020-09-05  3:41 ` Rich Felker
  2020-09-05  8:39 ` Stefan O'Rear
  0 siblings, 2 replies; 7+ messages in thread
From: Markus Wichmann @ 2020-09-04 19:52 UTC (permalink / raw)
  To: musl

Hi all,

now, the subject says "bug", but I don't think the conditions to trigger
this are even possible. But still, the code calls attention to it, so
here goes.

In ldso/dynlink.c there is a small function called mmap_fixed(). This
function contains this snippet:


|	ssize_t r;
|	if (lseek(fd, off, SEEK_SET) < 0) return MAP_FAILED;
|	for (q=p; n; q+=r, off+=r, n-=r) {
|		r = read(fd, q, n);
|		if (r < 0 && errno != EINTR) return MAP_FAILED;
|		if (!r) {
|			memset(q, 0, n);
|			break;
|		}
|	}

So when I read this, I immediately thought: What happens when the read()
call does fail due to EINTR? The code specifically excludes that error,
after all. The answer is that after EINTR, r is going to be -1, which
will not be corrected, so the iteration statements will actually back
off the target pointer and increase the remaining length. But since the
file position isn't also backed off, the results of that read will all
be shifted by one byte. If that happens the first time through the loop,
the code will also start overwriting one byte which it is not allowed to
touch (one byte in front of the buffer). I don't know, can this crash on
NOMMU systems? I am aware there were systems in the past lacking an MMU,
but having a memory protection unit. I just don't know if Linux runs on
any of them.

Because here's the crux of the issue: This code is unreachable on
anything but Super-H at the moment, since that is the only architecture
defining DL_NOMMU_SUPPORT. And it hinges on read() returning EINTR,
which, according to signal(7) is impossible: read() can only fail with
EINTR on devices where reading can block indefinitely, and those aren't
seekable. If someone did manage to push a file on the dynlinker where
this can happen, then the lseek() would fail already, and the rest of
the code would never run.

If I am right that the EINTR is impossible, it might be best to just
remove the exception for it.

Ciao,
Markus

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] Bug in mmap_fixed()
  2020-09-04 19:52 [musl] Bug in mmap_fixed() Markus Wichmann
@ 2020-09-05  3:41 ` Rich Felker
  2020-09-05  6:44   ` Markus Wichmann
  2020-09-05  8:39 ` Stefan O'Rear
  1 sibling, 1 reply; 7+ messages in thread
From: Rich Felker @ 2020-09-05  3:41 UTC (permalink / raw)
  To: musl

On Fri, Sep 04, 2020 at 09:52:51PM +0200, Markus Wichmann wrote:
> Hi all,
> 
> now, the subject says "bug", but I don't think the conditions to trigger
> this are even possible. But still, the code calls attention to it, so
> here goes.
> 
> In ldso/dynlink.c there is a small function called mmap_fixed(). This
> function contains this snippet:
> 
> 
> |	ssize_t r;
> |	if (lseek(fd, off, SEEK_SET) < 0) return MAP_FAILED;
> |	for (q=p; n; q+=r, off+=r, n-=r) {
> |		r = read(fd, q, n);
> |		if (r < 0 && errno != EINTR) return MAP_FAILED;
> |		if (!r) {
> |			memset(q, 0, n);
> |			break;
> |		}
> |	}
> 
> So when I read this, I immediately thought: What happens when the read()
> call does fail due to EINTR? The code specifically excludes that error,
> after all. The answer is that after EINTR, r is going to be -1, which
> will not be corrected, so the iteration statements will actually back
> off the target pointer and increase the remaining length. But since the
> file position isn't also backed off, the results of that read will all
> be shifted by one byte. If that happens the first time through the loop,
> the code will also start overwriting one byte which it is not allowed to
> touch (one byte in front of the buffer). I don't know, can this crash on
> NOMMU systems? I am aware there were systems in the past lacking an MMU,
> but having a memory protection unit. I just don't know if Linux runs on
> any of them.
> 
> Because here's the crux of the issue: This code is unreachable on
> anything but Super-H at the moment, since that is the only architecture
> defining DL_NOMMU_SUPPORT. And it hinges on read() returning EINTR,
> which, according to signal(7) is impossible: read() can only fail with
> EINTR on devices where reading can block indefinitely, and those aren't
> seekable. If someone did manage to push a file on the dynlinker where
> this can happen, then the lseek() would fail already, and the rest of
> the code would never run.
> 
> If I am right that the EINTR is impossible, it might be best to just
> remove the exception for it.

When I saw your report, I thought this code all ran with signals
blocked, and actually had to check to see that this isn't the case.
Note that for initial loading, there can be no signal dispositions
except SIG_DFL or SIG_IGN, so there is no EINTR, but it looks like it
can happen with dlopen if the application has installed interrupting
handlers. You missed the case where it can actually happen in
practice: NFS mounts configured to be interruptible rather than
hanging forever in uninterruptible sleep when the network goes down.

The code hsould be fixed, and EINTR handling should probably be left
in-place, just without the wrong pointer-advance logic.

Rich

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] Bug in mmap_fixed()
  2020-09-05  3:41 ` Rich Felker
@ 2020-09-05  6:44   ` Markus Wichmann
  2020-09-16  5:15     ` Rob Landley
  0 siblings, 1 reply; 7+ messages in thread
From: Markus Wichmann @ 2020-09-05  6:44 UTC (permalink / raw)
  To: musl

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

On Fri, Sep 04, 2020 at 11:41:54PM -0400, Rich Felker wrote:
> When I saw your report, I thought this code all ran with signals
> blocked, and actually had to check to see that this isn't the case.

In that case, making an exception for EINTR would be even weirder.

> The code hsould be fixed, and EINTR handling should probably be left
> in-place, just without the wrong pointer-advance logic.
>

See attached. Untested, obviously, since I lack a Super-H processor and
an NFS server, and even then the test case would be quite fiddly, but I
see nothing obviously wrong with it.

Ciao,
Markus

[-- Attachment #2: 0001-Fix-oversight-in-mmap_fixed.patch --]
[-- Type: text/x-diff, Size: 1097 bytes --]

From 3f1ab59a5db1f5c5d943c37981179621d44619d2 Mon Sep 17 00:00:00 2001
From: Markus Wichmann <nullplan@gmx.net>
Date: Sat, 5 Sep 2020 08:35:57 +0200
Subject: [PATCH] Fix oversight in mmap_fixed().

If the read() call in this function ever did return EINTR (which there
is an explicit exception for), then the pointers would be backed off by
one, resulting in the file contents being loaded in shifted by one byte.
And if that happens in the first run through the loop, one byte in front
of the destination buffer would be overwritten, which is invalid.
---
 ldso/dynlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index f7474743..51c4c004 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -576,7 +576,8 @@ static void *mmap_fixed(void *p, size_t n, int prot, int flags, int fd, off_t of
 	for (q=p; n; q+=r, off+=r, n-=r) {
 		r = read(fd, q, n);
 		if (r < 0 && errno != EINTR) return MAP_FAILED;
-		if (!r) {
+		else if (r < 0) r = 0;
+		else if (!r) {
 			memset(q, 0, n);
 			break;
 		}
--
2.17.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] Bug in mmap_fixed()
  2020-09-04 19:52 [musl] Bug in mmap_fixed() Markus Wichmann
  2020-09-05  3:41 ` Rich Felker
@ 2020-09-05  8:39 ` Stefan O'Rear
  1 sibling, 0 replies; 7+ messages in thread
From: Stefan O'Rear @ 2020-09-05  8:39 UTC (permalink / raw)
  To: musl

On Fri, Sep 4, 2020, at 3:52 PM, Markus Wichmann wrote:
> touch (one byte in front of the buffer). I don't know, can this crash on
> NOMMU systems? I am aware there were systems in the past lacking an MMU,
> but having a memory protection unit. I just don't know if Linux runs on
> any of them.

To answer the other question here, Linux runs on Cortex-M systems and has
code to program the memory protection unit; I was under the impression it could
do so on a per-process basis but I just checked arch/arm/mm/pmsa-v[78].c and it's
configured once at boot time.  There is an ARM FDPIC ABI but it is not implemented
in musl at this time.

Although even without precise bounds you could get a crash later if it overwrites
something important.

-s

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] Bug in mmap_fixed()
  2020-09-05  6:44   ` Markus Wichmann
@ 2020-09-16  5:15     ` Rob Landley
  2020-09-16 12:04       ` Rich Felker
  2020-09-16 14:20       ` Markus Wichmann
  0 siblings, 2 replies; 7+ messages in thread
From: Rob Landley @ 2020-09-16  5:15 UTC (permalink / raw)
  To: musl

On 9/5/20 1:44 AM, Markus Wichmann wrote:
> On Fri, Sep 04, 2020 at 11:41:54PM -0400, Rich Felker wrote:
>> When I saw your report, I thought this code all ran with signals
>> blocked, and actually had to check to see that this isn't the case.
> 
> In that case, making an exception for EINTR would be even weirder.
> 
>> The code hsould be fixed, and EINTR handling should probably be left
>> in-place, just without the wrong pointer-advance logic.
>>
> 
> See attached. Untested, obviously, since I lack a Super-H processor and
> an NFS server,

Coldfire is also nommu and musl has had m68k support for years, is there no
coldfire target? (That's been supported by qemu longer than proper m68k.)

Rob

P.S. This patches out the broken fork() on nommu sh2, and fixes the sh2 native
toolchain build:

  https://github.com/landley/toybox/blob/master/scripts/mcm-buildall.sh#L130

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] Bug in mmap_fixed()
  2020-09-16  5:15     ` Rob Landley
@ 2020-09-16 12:04       ` Rich Felker
  2020-09-16 14:20       ` Markus Wichmann
  1 sibling, 0 replies; 7+ messages in thread
From: Rich Felker @ 2020-09-16 12:04 UTC (permalink / raw)
  To: musl

On Wed, Sep 16, 2020 at 12:15:23AM -0500, Rob Landley wrote:
> On 9/5/20 1:44 AM, Markus Wichmann wrote:
> > On Fri, Sep 04, 2020 at 11:41:54PM -0400, Rich Felker wrote:
> >> When I saw your report, I thought this code all ran with signals
> >> blocked, and actually had to check to see that this isn't the case.
> > 
> > In that case, making an exception for EINTR would be even weirder.
> > 
> >> The code hsould be fixed, and EINTR handling should probably be left
> >> in-place, just without the wrong pointer-advance logic.
> >>
> > 
> > See attached. Untested, obviously, since I lack a Super-H processor and
> > an NFS server,
> 
> Coldfire is also nommu and musl has had m68k support for years, is there no
> coldfire target? (That's been supported by qemu longer than proper m68k.)
> 
> Rob
> 
> P.S. This patches out the broken fork() on nommu sh2, and fixes the sh2 native
> toolchain build:
> 
>   https://github.com/landley/toybox/blob/master/scripts/mcm-buildall.sh#L130

You have been told again and again this is not a bug, and your patch
is ABI breakage. There is no "nommu version of musl". There is a
common ABI that runs on mmuful and nommu machines and whether fork can
succeed or not is a runtime property.

Rich

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] Bug in mmap_fixed()
  2020-09-16  5:15     ` Rob Landley
  2020-09-16 12:04       ` Rich Felker
@ 2020-09-16 14:20       ` Markus Wichmann
  1 sibling, 0 replies; 7+ messages in thread
From: Markus Wichmann @ 2020-09-16 14:20 UTC (permalink / raw)
  To: musl

On Wed, Sep 16, 2020 at 12:15:23AM -0500, Rob Landley wrote:
> Coldfire is also nommu and musl has had m68k support for years, is there no
> coldfire target? (That's been supported by qemu longer than proper m68k.)

As stated in the OP, whether the buggy code even runs depends on
DL_NOMMU_SUPPORT, which as of yet only Super-H defines to 1. Even
if the buggy code runs, it will only bug out if a read() on a shared
library fails with EINTR, which is impossible during initial load (since
no signal handler can be installed), and almost impossible at runtime,
since shared libraries tend to be regular files, and only regular files
on an NFS share that has been setup to do interrupting I/O can ever
return EINTR. Even then, the first read() (that reads the ELF header and
the program headers) has to go off without a hitch, or else we won't
even come that far.

As far as I can tell, there is no special Coldfire target. If Coldfire
is incompatible with the M68k ABI (and from what I've just read, they
stripped a lot of features out of M68k to make Coldfire), then no, there
is no support for Coldfire.

Ciao,
Markus

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-09-16 14:20 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 19:52 [musl] Bug in mmap_fixed() Markus Wichmann
2020-09-05  3:41 ` Rich Felker
2020-09-05  6:44   ` Markus Wichmann
2020-09-16  5:15     ` Rob Landley
2020-09-16 12:04       ` Rich Felker
2020-09-16 14:20       ` Markus Wichmann
2020-09-05  8:39 ` Stefan O'Rear

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).