From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x532.google.com (mail-pg1-x532.google.com [IPv6:2607:f8b0:4864:20::532]) by sourceware.org (Postfix) with ESMTPS id 1CC443858D35 for ; Fri, 5 Nov 2021 14:27:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1CC443858D35 Received: by mail-pg1-x532.google.com with SMTP id p8so7188797pgh.11 for ; Fri, 05 Nov 2021 07:27:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XHm+rJ929Go8GYIbAb6uGuBhNV5NDH4XWZRIMVUJG1U=; b=a9YrQhD5NAam19ITul+vESMcUT6I7t8fjALFkiS1UebE2dGr2HNYX58lUNfCi5Ao9r 06z7nuGJ2Nok3X5J1G3b/Ib0MqKJo5hwq65tP8h/XtLmGB0UIjOOJjZJ6E22LQHEG+30 A4N5xNXjHQbjjRDiY3k1tRmAY3pWxw3JVNKpjcIKFc8Ewh8scq75sG5XL8AS73iQf9NP GdDye2cuzA/Pv/6Rp7MLo7VSk83ohramzVcmdfXyeo8Lhps1enD9+MHsMYjqOGOjkTjM fkPpTDk64A3qaEG/HzBY3upGRFwqqYqtR0yYNHnHZtXu28GKsP2ccy2UWwhd4x5CKMvK np2w== X-Gm-Message-State: AOAM530yveNIUXu9DGBYPcnMxmYd4sldLOks4bNJlQ0I6duuYZ3lhacY V354E+riVg51Zl0k4xUtTsy+4nE2h5FPexlatero6SN4hxM= X-Google-Smtp-Source: ABdhPJzI140ljlJTw6dS+X1U07ZWtXT9Jehd+j50D9vPHQcmYIQPm5UlSybT5yLaGY4uwV8IwZCl6kptNmtitV7WdPw= X-Received: by 2002:a05:6a00:22cb:b0:494:6363:c44f with SMTP id f11-20020a056a0022cb00b004946363c44fmr11859823pfj.43.1636122443101; Fri, 05 Nov 2021 07:27:23 -0700 (PDT) MIME-Version: 1.0 References: <27f078539ae2a5b390705ac6fa1a7437ae8ce97c.1636120354.git.fweimer@redhat.com> In-Reply-To: <27f078539ae2a5b390705ac6fa1a7437ae8ce97c.1636120354.git.fweimer@redhat.com> From: "H.J. Lu" Date: Fri, 5 Nov 2021 07:26:47 -0700 Message-ID: Subject: Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading To: Florian Weimer Cc: GNU C Library Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3030.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Nov 2021 14:27:25 -0000 On Fri, Nov 5, 2021 at 7:01 AM Florian Weimer via Libc-alpha wrote: > > We occasionally see elf/tst-debug1 failing with a SIGBUS error > with some toolchain versions (recently on aarch64 only). This test > tries to emulate loading separated debuginfo, but whether the test > object triggers the crash depends on many factors. Accurately > rejected separated debuginfo in dlopen probably needs ELF markup, > at least if this is to be done efficiently using program headers > only. But this change still improves user experience slightly. > We already obtain the file size from the kernel in most cases, > so no additional system call is added. Under what conditions does the test trigger SIGBUS? Does your patch makes the test pass or just turn SIGBUS into a different failure? > > Based on earlier downstream-only patch by Jeff Law. > --- > elf/dl-load.c | 78 +++++++++++++++++++++---------------- > sysdeps/generic/dl-fileid.h | 7 ++-- > 2 files changed, 49 insertions(+), 36 deletions(-) > > diff --git a/elf/dl-load.c b/elf/dl-load.c > index a1f1682188..a758bed9af 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -953,47 +953,48 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, > struct r_debug *r = _dl_debug_update (nsid); > bool make_consistent = false; > > - /* Get file information. To match the kernel behavior, do not fill > - in this information for the executable in case of an explicit > - loader invocation. */ > + /* Get file information. */ > struct r_file_id id; > + off64_t file_size; > + if (__glibc_unlikely (!_dl_get_file_id (fd, &id, &file_size))) > + { > + errstring = N_("cannot stat shared object"); > + lose_errno: > + errval = errno; > + lose: > + /* The file might already be closed. */ > + if (fd != -1) > + __close_nocancel (fd); > + if (l != NULL && l->l_map_start != 0) > + _dl_unmap_segments (l); > + if (l != NULL && l->l_origin != (char *) -1l) > + free ((char *) l->l_origin); > + if (l != NULL && !l->l_libname->dont_free) > + free (l->l_libname); > + if (l != NULL && l->l_phdr_allocated) > + free ((void *) l->l_phdr); > + free (l); > + free (realname); > + > + if (make_consistent && r != NULL) > + { > + r->r_state = RT_CONSISTENT; > + _dl_debug_state (); > + LIBC_PROBE (map_failed, 2, nsid, r); > + } > + > + _dl_signal_error (errval, name, NULL, errstring); > + } > + > if (mode & __RTLD_OPENEXEC) > { > assert (nsid == LM_ID_BASE); > + /* To match the kernel behavior, do not fill in this information > + for the executable in case of an explicit loader invocation. */ > memset (&id, 0, sizeof (id)); > } > else > { > - if (__glibc_unlikely (!_dl_get_file_id (fd, &id))) > - { > - errstring = N_("cannot stat shared object"); > - lose_errno: > - errval = errno; > - lose: > - /* The file might already be closed. */ > - if (fd != -1) > - __close_nocancel (fd); > - if (l != NULL && l->l_map_start != 0) > - _dl_unmap_segments (l); > - if (l != NULL && l->l_origin != (char *) -1l) > - free ((char *) l->l_origin); > - if (l != NULL && !l->l_libname->dont_free) > - free (l->l_libname); > - if (l != NULL && l->l_phdr_allocated) > - free ((void *) l->l_phdr); > - free (l); > - free (realname); > - > - if (make_consistent && r != NULL) > - { > - r->r_state = RT_CONSISTENT; > - _dl_debug_state (); > - LIBC_PROBE (map_failed, 2, nsid, r); > - } > - > - _dl_signal_error (errval, name, NULL, errstring); > - } > - > /* Look again to see if the real name matched another already loaded. */ > for (l = GL(dl_ns)[nsid]._ns_loaded; l != NULL; l = l->l_next) > if (!l->l_removed && _dl_file_id_match_p (&l->l_file_id, &id)) > @@ -1177,6 +1178,17 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, > = N_("ELF load command address/offset not properly aligned"); > goto lose; > } > + if (__glibc_unlikely (ph->p_offset + ph->p_filesz > file_size)) > + { > + /* If the segment is not fully backed by the file, > + accessing memory beyond the last full page results in > + SIGBUS. This often happens with non-loadable ELF > + objects containing separated debugging information > + (which have load segments that match the original ELF > + file). */ > + errstring = N_("ELF load command past end of file"); > + goto lose; > + } > > struct loadcmd *c = &loadcmds[nloadcmds++]; > c->mapstart = ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize)); > diff --git a/sysdeps/generic/dl-fileid.h b/sysdeps/generic/dl-fileid.h > index bf437f3d71..c59627429c 100644 > --- a/sysdeps/generic/dl-fileid.h > +++ b/sysdeps/generic/dl-fileid.h > @@ -27,10 +27,10 @@ struct r_file_id > ino64_t ino; > }; > > -/* Sample FD to fill in *ID. Returns true on success. > - On error, returns false, with errno set. */ > +/* Sample FD to fill in *ID, *SIZE. Returns true on success. On > + error, returns false, with errno set. */ > static inline bool > -_dl_get_file_id (int fd, struct r_file_id *id) > +_dl_get_file_id (int fd, struct r_file_id *id, off64_t *size) > { > struct __stat64_t64 st; > > @@ -39,6 +39,7 @@ _dl_get_file_id (int fd, struct r_file_id *id) > > id->dev = st.st_dev; > id->ino = st.st_ino; > + *size = st.st_size; > return true; > } > > -- > 2.33.1 > -- H.J.