Re: [musl] Bug in mmap_fixed()

[Rich Felker <dalias@libc.org>]

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