Message ID | CAMe9rOoAQgbtMzftq6UOG_dMvUL3EtAu2Gk3bu_7=Rdt27F+qw@mail.gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 74980 invoked by alias); 27 Jul 2018 18:22:04 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 74961 invoked by uid 89); 27 Jul 2018 18:22:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=H*r:sk:y207-v6 X-HELO: mail-oi0-f67.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:from:date:message-id:subject:to:cc; bh=7zzeo9ep90jQ2VJ7zVJHLlHy91GjZbJ7BI0Kb7PUvRg=; b=dSAtaLaxZQhLFUrk0deVa0+1janUmaemKmOYTNmZr+zOVMFlEhyhLP9Aj1+QFX9lIK +trt83hb4RNPwc6P+48BjSt7iDE/gBJdDW6MV3rk84tcGE0xux4+8Bunc7oKXfoZ4JAt bGiJh6bDhkVBd/GAXvetKFT6aaFoUWTMuEuWQvXr48donGIm3xeX5uQBRl7qrLeIiYwJ ApbOgmzoXQ0RJw8/ZxyVfhNWl7XjTF8aV2+1L91N5Y77+OPD1KwEAXov/gLeOI2FxAXq w55iHyDTqoZUoTnbftSorZ+L88pMC2m4LerG7ttfpFS+j3OjQQp59zRlqK3xPChVn6Ix rvdw== MIME-Version: 1.0 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Fri, 27 Jul 2018 11:22:00 -0700 Message-ID: <CAMe9rOoAQgbtMzftq6UOG_dMvUL3EtAu2Gk3bu_7=Rdt27F+qw@mail.gmail.com> Subject: [PATCH] x86/CET: Don't parse beyond the note end To: Florian Weimer <fweimer@redhat.com> Cc: GNU C Library <libc-alpha@sourceware.org>, "Carlos O'Donell" <carlos@redhat.com> Content-Type: multipart/mixed; boundary="0000000000001b45f70571ff30ab" |
Commit Message
H.J. Lu
July 27, 2018, 6:22 p.m. UTC
On Fri, Jul 27, 2018 at 11:20 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 07/27/2018 07:56 PM, H.J. Lu wrote: >> >> Yes, I can reproduce it. Let me take a look. > > > Great. Did you see the patch I posted? > Please this one instead.
Comments
On 07/27/2018 08:22 PM, H.J. Lu wrote: > - while (1) > + while (ptr < ptr_end) > { > unsigned int type = *(unsigned int *) ptr; > unsigned int datasz = *(unsigned int *) (ptr + 4); You need 1 byte, but 8 bytes. Why is checking for at least 1 byte sufficient here? I suppose our position is that we do not care about corrupt binaries. Is that justification for writing the check this way? Thanks, Florian
On Fri, Jul 27, 2018 at 11:26 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 07/27/2018 08:22 PM, H.J. Lu wrote: >> >> - while (1) >> + while (ptr < ptr_end) >> { >> unsigned int type = *(unsigned int *) ptr; >> unsigned int datasz = *(unsigned int *) (ptr + 4); > > > You need 1 byte, but 8 bytes. Why is checking for at least 1 byte > sufficient here? > There is: /* Check for invalid property. */ if (note->n_descsz < 8 || (note->n_descsz % sizeof (ElfW(Addr))) != 0) break; before that. n_descsz should be correct.
On 07/27/2018 02:22 PM, H.J. Lu wrote: > On Fri, Jul 27, 2018 at 11:20 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 07/27/2018 07:56 PM, H.J. Lu wrote: >>> Yes, I can reproduce it. Let me take a look. >> >> Great. Did you see the patch I posted? >> > Please this one instead. > > > -- H.J. > > > 0001-x86-CET-Don-t-parse-beyond-the-note-end.patch > > > From 8de773a7f9225bb9e42eae1263719ca506670087 Mon Sep 17 00:00:00 2001 > From: "H.J. Lu" <hjl.tools@gmail.com> > Date: Fri, 27 Jul 2018 11:17:04 -0700 > Subject: [PATCH] x86/CET: Don't parse beyond the note end > > Simply check if "ptr < ptr_end" since "ptr" is always incremented by 8. > > * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Don't > parse beyond the note end. This is OK for 2.28, limited to x86, and so low impact to other machine testing going on. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- > sysdeps/x86/dl-prop.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h > index d56e20a6dc..35d3f16a23 100644 > --- a/sysdeps/x86/dl-prop.h > +++ b/sysdeps/x86/dl-prop.h > @@ -73,7 +73,7 @@ _dl_process_cet_property_note (struct link_map *l, > unsigned char *ptr = (unsigned char *) (note + 1) + 4; > unsigned char *ptr_end = ptr + note->n_descsz; > > - while (1) > + while (ptr < ptr_end) OK. This only happens with a note that is large enough not to be considered invalid, but not large enough to contain the data we need. So when we read the start of the property array, we are already out of bounds. Is this an empty property array? > { > unsigned int type = *(unsigned int *) ptr; > unsigned int datasz = *(unsigned int *) (ptr + 4); > -- 2.17.1 Cheers, Carlos.
On Fri, Jul 27, 2018 at 12:22 PM, Carlos O'Donell <carlos@redhat.com> wrote: > On 07/27/2018 02:22 PM, H.J. Lu wrote: >> On Fri, Jul 27, 2018 at 11:20 AM, Florian Weimer <fweimer@redhat.com> wrote: >>> On 07/27/2018 07:56 PM, H.J. Lu wrote: >>>> Yes, I can reproduce it. Let me take a look. >>> >>> Great. Did you see the patch I posted? >>> >> Please this one instead. >> >> >> -- H.J. >> >> >> 0001-x86-CET-Don-t-parse-beyond-the-note-end.patch >> >> >> From 8de773a7f9225bb9e42eae1263719ca506670087 Mon Sep 17 00:00:00 2001 >> From: "H.J. Lu" <hjl.tools@gmail.com> >> Date: Fri, 27 Jul 2018 11:17:04 -0700 >> Subject: [PATCH] x86/CET: Don't parse beyond the note end >> >> Simply check if "ptr < ptr_end" since "ptr" is always incremented by 8. >> >> * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Don't >> parse beyond the note end. > > This is OK for 2.28, limited to x86, and so low impact to other machine > testing going on. > > Reviewed-by: Carlos O'Donell <carlos@redhat.com> > >> --- >> sysdeps/x86/dl-prop.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h >> index d56e20a6dc..35d3f16a23 100644 >> --- a/sysdeps/x86/dl-prop.h >> +++ b/sysdeps/x86/dl-prop.h >> @@ -73,7 +73,7 @@ _dl_process_cet_property_note (struct link_map *l, >> unsigned char *ptr = (unsigned char *) (note + 1) + 4; >> unsigned char *ptr_end = ptr + note->n_descsz; >> >> - while (1) >> + while (ptr < ptr_end) > > OK. This only happens with a note that is large enough not to be > considered invalid, but not large enough to contain the data we > need. So when we read the start of the property array, we are > already out of bounds. > > Is this an empty property array? It has Displaying notes found in: .note.gnu.property Owner Data size Description GNU 0x00000020 NT_GNU_PROPERTY_TYPE_0 Properties: x86 ISA used: x86 ISA needed: It has a property with all bits cleared. I checked in a linker patch: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=56ad703d56ffe5dc55d5e719a6ec41fd6cf9bfbe to remove such property. It should be fixed for binutils 2.31.
On 07/27/2018 08:47 PM, H.J. Lu wrote: > On Fri, Jul 27, 2018 at 11:26 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 07/27/2018 08:22 PM, H.J. Lu wrote: >>> >>> - while (1) >>> + while (ptr < ptr_end) >>> { >>> unsigned int type = *(unsigned int *) ptr; >>> unsigned int datasz = *(unsigned int *) (ptr + 4); >> >> >> You need 1 byte, but 8 bytes. Why is checking for at least 1 byte >> sufficient here? >> > > There is: > > /* Check for invalid property. */ > if (note->n_descsz < 8 > || (note->n_descsz % sizeof (ElfW(Addr))) != 0) > break; > > before that. n_descsz should be correct. I do not have a strong opinion regarding this matter. For correctly generated notes, your patch should be fine. Thanks, Florian
From 8de773a7f9225bb9e42eae1263719ca506670087 Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Fri, 27 Jul 2018 11:17:04 -0700 Subject: [PATCH] x86/CET: Don't parse beyond the note end Simply check if "ptr < ptr_end" since "ptr" is always incremented by 8. * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Don't parse beyond the note end. --- sysdeps/x86/dl-prop.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h index d56e20a6dc..35d3f16a23 100644 --- a/sysdeps/x86/dl-prop.h +++ b/sysdeps/x86/dl-prop.h @@ -73,7 +73,7 @@ _dl_process_cet_property_note (struct link_map *l, unsigned char *ptr = (unsigned char *) (note + 1) + 4; unsigned char *ptr_end = ptr + note->n_descsz; - while (1) + while (ptr < ptr_end) { unsigned int type = *(unsigned int *) ptr; unsigned int datasz = *(unsigned int *) (ptr + 4); -- 2.17.1