From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 6246 invoked from network); 5 Sep 2020 03:42:10 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 5 Sep 2020 03:42:10 -0000 Received: (qmail 28652 invoked by uid 550); 5 Sep 2020 03:42:07 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 28632 invoked from network); 5 Sep 2020 03:42:06 -0000 Date: Fri, 4 Sep 2020 23:41:54 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20200905034153.GI3265@brightrain.aerifal.cx> References: <20200904195251.GA2139@voyager> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200904195251.GA2139@voyager> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] Bug in mmap_fixed() 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