Bug in mmap_fixed()

Submitted by Markus Wichmann on Sept. 5, 2020, 6:44 a.m.

Details

Message ID 20200905064419.GB2139@voyager
State New
Series "Bug in mmap_fixed()"
Headers show

Commit Message

Markus Wichmann Sept. 5, 2020, 6:44 a.m.
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

Patch hide | download patch | download mbox

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


Comments

Rob Landley Sept. 16, 2020, 5:15 a.m.
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
Rich Felker Sept. 16, 2020, 12:04 p.m.
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
Markus Wichmann Sept. 16, 2020, 2:20 p.m.
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