| Message ID | 20230915204234.3547739-1-martin.jansa@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [mickledore] gcc: backport a fix for ICE caused by CVE-2023-4039.patch | expand |
The CVE-2023-4039.patch was also submitted for kirkstone and dunfell. Should I attempt to cherry-pick this fix for those branches too? Steve On Fri, Sep 15, 2023 at 10:42 AM Martin Jansa <martin.jansa@gmail.com> wrote: > > * see: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111418 > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111411 > > Signed-off-by: Martin Jansa <martin.jansa@gmail.com> > --- > meta/recipes-devtools/gcc/gcc-12.3.inc | 1 + > ...ch64-Fix-loose-ldpstp-check-PR111411.patch | 117 ++++++++++++++++++ > 2 files changed, 118 insertions(+) > create mode 100644 meta/recipes-devtools/gcc/gcc/0026-aarch64-Fix-loose-ldpstp-check-PR111411.patch > > diff --git a/meta/recipes-devtools/gcc/gcc-12.3.inc b/meta/recipes-devtools/gcc/gcc-12.3.inc > index 5896f26e1a..5655b6f46d 100644 > --- a/meta/recipes-devtools/gcc/gcc-12.3.inc > +++ b/meta/recipes-devtools/gcc/gcc-12.3.inc > @@ -64,6 +64,7 @@ SRC_URI = "${BASEURI} \ > file://prefix-map-realpath.patch \ > file://hardcoded-paths.patch \ > file://CVE-2023-4039.patch \ > + file://0026-aarch64-Fix-loose-ldpstp-check-PR111411.patch \ > " > SRC_URI[sha256sum] = "949a5d4f99e786421a93b532b22ffab5578de7321369975b91aec97adfda8c3b" > > diff --git a/meta/recipes-devtools/gcc/gcc/0026-aarch64-Fix-loose-ldpstp-check-PR111411.patch b/meta/recipes-devtools/gcc/gcc/0026-aarch64-Fix-loose-ldpstp-check-PR111411.patch > new file mode 100644 > index 0000000000..a408a98698 > --- /dev/null > +++ b/meta/recipes-devtools/gcc/gcc/0026-aarch64-Fix-loose-ldpstp-check-PR111411.patch > @@ -0,0 +1,117 @@ > +From adb60dc78e0da4877747f32347cee339364775be Mon Sep 17 00:00:00 2001 > +From: Richard Sandiford <richard.sandiford@arm.com> > +Date: Fri, 15 Sep 2023 09:19:14 +0100 > +Subject: [PATCH] aarch64: Fix loose ldpstp check [PR111411] > + > +aarch64_operands_ok_for_ldpstp contained the code: > + > + /* One of the memory accesses must be a mempair operand. > + If it is not the first one, they need to be swapped by the > + peephole. */ > + if (!aarch64_mem_pair_operand (mem_1, GET_MODE (mem_1)) > + && !aarch64_mem_pair_operand (mem_2, GET_MODE (mem_2))) > + return false; > + > +But the requirement isn't just that one of the accesses must be a > +valid mempair operand. It's that the lower access must be, since > +that's the access that will be used for the instruction operand. > + > +gcc/ > + PR target/111411 > + * config/aarch64/aarch64.cc (aarch64_operands_ok_for_ldpstp): Require > + the lower memory access to a mem-pair operand. > + > +gcc/testsuite/ > + PR target/111411 > + * gcc.dg/rtl/aarch64/pr111411.c: New test. > + > +Upstream-Status: Backport [https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=2d38f45bcca62ca0c7afef4b579f82c5c2a01610] > +Signed-off-by: Martin Jansa <martin.jansa@gmail.com> > +--- > + gcc/config/aarch64/aarch64.cc | 8 ++- > + gcc/testsuite/gcc.dg/rtl/aarch64/pr111411.c | 57 +++++++++++++++++++++ > + 2 files changed, 60 insertions(+), 5 deletions(-) > + create mode 100644 gcc/testsuite/gcc.dg/rtl/aarch64/pr111411.c > + > +diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > +index 6118a3354ac..9b1f791ca8b 100644 > +--- a/gcc/config/aarch64/aarch64.cc > ++++ b/gcc/config/aarch64/aarch64.cc > +@@ -26154,11 +26154,9 @@ aarch64_operands_ok_for_ldpstp (rtx *operands, bool load, > + gcc_assert (known_eq (GET_MODE_SIZE (GET_MODE (mem_1)), > + GET_MODE_SIZE (GET_MODE (mem_2)))); > + > +- /* One of the memory accesses must be a mempair operand. > +- If it is not the first one, they need to be swapped by the > +- peephole. */ > +- if (!aarch64_mem_pair_operand (mem_1, GET_MODE (mem_1)) > +- && !aarch64_mem_pair_operand (mem_2, GET_MODE (mem_2))) > ++ /* The lower memory access must be a mem-pair operand. */ > ++ rtx lower_mem = reversed ? mem_2 : mem_1; > ++ if (!aarch64_mem_pair_operand (lower_mem, GET_MODE (lower_mem))) > + return false; > + > + if (REG_P (reg_1) && FP_REGNUM_P (REGNO (reg_1))) > +diff --git a/gcc/testsuite/gcc.dg/rtl/aarch64/pr111411.c b/gcc/testsuite/gcc.dg/rtl/aarch64/pr111411.c > +new file mode 100644 > +index 00000000000..ad07e9c6c89 > +--- /dev/null > ++++ b/gcc/testsuite/gcc.dg/rtl/aarch64/pr111411.c > +@@ -0,0 +1,57 @@ > ++/* { dg-do compile { target aarch64*-*-* } } */ > ++/* { dg-require-effective-target lp64 } */ > ++/* { dg-options "-O -fdisable-rtl-postreload -fpeephole2 -fno-schedule-fusion" } */ > ++ > ++extern int data[]; > ++ > ++void __RTL (startwith ("ira")) foo (void *ptr) > ++{ > ++ (function "foo" > ++ (param "ptr" > ++ (DECL_RTL (reg/v:DI <0> [ ptr ])) > ++ (DECL_RTL_INCOMING (reg/v:DI x0 [ ptr ])) > ++ ) ;; param "ptr" > ++ (insn-chain > ++ (block 2 > ++ (edge-from entry (flags "FALLTHRU")) > ++ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK) > ++ (insn 4 (set (reg:DI <0>) (reg:DI x0))) > ++ (insn 5 (set (reg:DI <1>) > ++ (plus:DI (reg:DI <0>) (const_int 768)))) > ++ (insn 6 (set (mem:SI (plus:DI (reg:DI <0>) > ++ (const_int 508)) [1 &data+508 S4 A4]) > ++ (const_int 0))) > ++ (insn 7 (set (mem:SI (plus:DI (reg:DI <1>) > ++ (const_int -256)) [1 &data+512 S4 A4]) > ++ (const_int 0))) > ++ (edge-to exit (flags "FALLTHRU")) > ++ ) ;; block 2 > ++ ) ;; insn-chain > ++ ) ;; function > ++} > ++ > ++void __RTL (startwith ("ira")) bar (void *ptr) > ++{ > ++ (function "bar" > ++ (param "ptr" > ++ (DECL_RTL (reg/v:DI <0> [ ptr ])) > ++ (DECL_RTL_INCOMING (reg/v:DI x0 [ ptr ])) > ++ ) ;; param "ptr" > ++ (insn-chain > ++ (block 2 > ++ (edge-from entry (flags "FALLTHRU")) > ++ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK) > ++ (insn 4 (set (reg:DI <0>) (reg:DI x0))) > ++ (insn 5 (set (reg:DI <1>) > ++ (plus:DI (reg:DI <0>) (const_int 768)))) > ++ (insn 6 (set (mem:SI (plus:DI (reg:DI <1>) > ++ (const_int -256)) [1 &data+512 S4 A4]) > ++ (const_int 0))) > ++ (insn 7 (set (mem:SI (plus:DI (reg:DI <0>) > ++ (const_int 508)) [1 &data+508 S4 A4]) > ++ (const_int 0))) > ++ (edge-to exit (flags "FALLTHRU")) > ++ ) ;; block 2 > ++ ) ;; insn-chain > ++ ) ;; function > ++} > -- > 2.42.0 > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#187768): https://lists.openembedded.org/g/openembedded-core/message/187768 > Mute This Topic: https://lists.openembedded.org/mt/101387993/3620601 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [steve@sakoman.com] > -=-=-=-=-=-=-=-=-=-=-=- >
On 18 Sep 2023, at 16:48, Steve Sakoman <steve@sakoman.com> wrote: > > The CVE-2023-4039.patch was also submitted for kirkstone and dunfell. > Should I attempt to cherry-pick this fix for those branches too? The bug says that the patch only applies cleaning to GCC 12/13, I’m seeking clarification as to whether that means it’s not relevant for older branches or whether a proper backport will be needed. Ross
On Mon, Sep 18, 2023 at 6:08 AM Ross Burton <Ross.Burton@arm.com> wrote: > > On 18 Sep 2023, at 16:48, Steve Sakoman <steve@sakoman.com> wrote: > > > > The CVE-2023-4039.patch was also submitted for kirkstone and dunfell. > > Should I attempt to cherry-pick this fix for those branches too? > > The bug says that the patch only applies cleaning to GCC 12/13, I’m seeking clarification as to whether that means it’s not relevant for older branches or whether a proper backport will be needed. I did a quick experiment with kirkstone and there were two issues with cherry-picking: 1. The test suite hunk failed since the test file doesn't exist in gcc 11 2. The first hunk fails to apply since the file name being modified in gcc 11 is a .c, apparently it was changed to .cc in later releases. You took this into account in the CVE patch file Removing the testsuite hunk and hacking the filename allowed the first hunk to apply (but with fuzz errors). So I'd say we definitely need a proper backport if the issue is relevant for older branches. I'll pull CVE-2023-4039.patch from kirkstone and dunfell until we have clarification. Thanks! Steve
I've intentionally sent it only for mickledore as the gcc version in kirkstone and dunfell isn't affected by this (at least my reproducer isn't). On Mon, Sep 18, 2023 at 6:42 PM Steve Sakoman <steve@sakoman.com> wrote: > On Mon, Sep 18, 2023 at 6:08 AM Ross Burton <Ross.Burton@arm.com> wrote: > > > > On 18 Sep 2023, at 16:48, Steve Sakoman <steve@sakoman.com> wrote: > > > > > > The CVE-2023-4039.patch was also submitted for kirkstone and dunfell. > > > Should I attempt to cherry-pick this fix for those branches too? > > > > The bug says that the patch only applies cleaning to GCC 12/13, I’m > seeking clarification as to whether that means it’s not relevant for older > branches or whether a proper backport will be needed. > > I did a quick experiment with kirkstone and there were two issues with > cherry-picking: > > 1. The test suite hunk failed since the test file doesn't exist in gcc 11 > 2. The first hunk fails to apply since the file name being modified in > gcc 11 is a .c, apparently it was changed to .cc in later releases. > You took this into account in the CVE patch file > > Removing the testsuite hunk and hacking the filename allowed the first > hunk to apply (but with fuzz errors). > > So I'd say we definitely need a proper backport if the issue is > relevant for older branches. > > I'll pull CVE-2023-4039.patch from kirkstone and dunfell until we have > clarification. > > Thanks! > > Steve >
On Mon, Sep 18, 2023 at 7:10 AM Martin Jansa <martin.jansa@gmail.com> wrote: > > I've intentionally sent it only for mickledore as the gcc version in kirkstone and dunfell isn't affected by this (at least my reproducer isn't). So, just to clarify, is it OK to take the CVE-2023-4039.patch for kirkstone and dunfell? Steve > > On Mon, Sep 18, 2023 at 6:42 PM Steve Sakoman <steve@sakoman.com> wrote: >> >> On Mon, Sep 18, 2023 at 6:08 AM Ross Burton <Ross.Burton@arm.com> wrote: >> > >> > On 18 Sep 2023, at 16:48, Steve Sakoman <steve@sakoman.com> wrote: >> > > >> > > The CVE-2023-4039.patch was also submitted for kirkstone and dunfell. >> > > Should I attempt to cherry-pick this fix for those branches too? >> > >> > The bug says that the patch only applies cleaning to GCC 12/13, I’m seeking clarification as to whether that means it’s not relevant for older branches or whether a proper backport will be needed. >> >> I did a quick experiment with kirkstone and there were two issues with >> cherry-picking: >> >> 1. The test suite hunk failed since the test file doesn't exist in gcc 11 >> 2. The first hunk fails to apply since the file name being modified in >> gcc 11 is a .c, apparently it was changed to .cc in later releases. >> You took this into account in the CVE patch file >> >> Removing the testsuite hunk and hacking the filename allowed the first >> hunk to apply (but with fuzz errors). >> >> So I'd say we definitely need a proper backport if the issue is >> relevant for older branches. >> >> I'll pull CVE-2023-4039.patch from kirkstone and dunfell until we have >> clarification. >> >> Thanks! >> >> Steve
On Mon, Sep 18, 2023 at 7:21 PM Steve Sakoman <steve@sakoman.com> wrote: > On Mon, Sep 18, 2023 at 7:10 AM Martin Jansa <martin.jansa@gmail.com> > wrote: > > > > I've intentionally sent it only for mickledore as the gcc version in > kirkstone and dunfell isn't affected by this (at least my reproducer isn't). > > So, just to clarify, is it OK to take the CVE-2023-4039.patch for > kirkstone and dunfell? > I think so.
On 18 Sep 2023, at 18:50, Martin Jansa <martin.jansa@gmail.com> wrote: > > On Mon, Sep 18, 2023 at 7:21 PM Steve Sakoman <steve@sakoman.com> wrote: > On Mon, Sep 18, 2023 at 7:10 AM Martin Jansa <martin.jansa@gmail.com> wrote: > > > > I've intentionally sent it only for mickledore as the gcc version in kirkstone and dunfell isn't affected by this (at least my reproducer isn't). > > So, just to clarify, is it OK to take the CVE-2023-4039.patch for > kirkstone and dunfell? > > I think so. Agreed. The relevant code changed a fair bit and there’s no evidence right now that it was broken in older releases. Ross
diff --git a/meta/recipes-devtools/gcc/gcc-12.3.inc b/meta/recipes-devtools/gcc/gcc-12.3.inc index 5896f26e1a..5655b6f46d 100644 --- a/meta/recipes-devtools/gcc/gcc-12.3.inc +++ b/meta/recipes-devtools/gcc/gcc-12.3.inc @@ -64,6 +64,7 @@ SRC_URI = "${BASEURI} \ file://prefix-map-realpath.patch \ file://hardcoded-paths.patch \ file://CVE-2023-4039.patch \ + file://0026-aarch64-Fix-loose-ldpstp-check-PR111411.patch \ " SRC_URI[sha256sum] = "949a5d4f99e786421a93b532b22ffab5578de7321369975b91aec97adfda8c3b" diff --git a/meta/recipes-devtools/gcc/gcc/0026-aarch64-Fix-loose-ldpstp-check-PR111411.patch b/meta/recipes-devtools/gcc/gcc/0026-aarch64-Fix-loose-ldpstp-check-PR111411.patch new file mode 100644 index 0000000000..a408a98698 --- /dev/null +++ b/meta/recipes-devtools/gcc/gcc/0026-aarch64-Fix-loose-ldpstp-check-PR111411.patch @@ -0,0 +1,117 @@ +From adb60dc78e0da4877747f32347cee339364775be Mon Sep 17 00:00:00 2001 +From: Richard Sandiford <richard.sandiford@arm.com> +Date: Fri, 15 Sep 2023 09:19:14 +0100 +Subject: [PATCH] aarch64: Fix loose ldpstp check [PR111411] + +aarch64_operands_ok_for_ldpstp contained the code: + + /* One of the memory accesses must be a mempair operand. + If it is not the first one, they need to be swapped by the + peephole. */ + if (!aarch64_mem_pair_operand (mem_1, GET_MODE (mem_1)) + && !aarch64_mem_pair_operand (mem_2, GET_MODE (mem_2))) + return false; + +But the requirement isn't just that one of the accesses must be a +valid mempair operand. It's that the lower access must be, since +that's the access that will be used for the instruction operand. + +gcc/ + PR target/111411 + * config/aarch64/aarch64.cc (aarch64_operands_ok_for_ldpstp): Require + the lower memory access to a mem-pair operand. + +gcc/testsuite/ + PR target/111411 + * gcc.dg/rtl/aarch64/pr111411.c: New test. + +Upstream-Status: Backport [https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=2d38f45bcca62ca0c7afef4b579f82c5c2a01610] +Signed-off-by: Martin Jansa <martin.jansa@gmail.com> +--- + gcc/config/aarch64/aarch64.cc | 8 ++- + gcc/testsuite/gcc.dg/rtl/aarch64/pr111411.c | 57 +++++++++++++++++++++ + 2 files changed, 60 insertions(+), 5 deletions(-) + create mode 100644 gcc/testsuite/gcc.dg/rtl/aarch64/pr111411.c + +diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc +index 6118a3354ac..9b1f791ca8b 100644 +--- a/gcc/config/aarch64/aarch64.cc ++++ b/gcc/config/aarch64/aarch64.cc +@@ -26154,11 +26154,9 @@ aarch64_operands_ok_for_ldpstp (rtx *operands, bool load, + gcc_assert (known_eq (GET_MODE_SIZE (GET_MODE (mem_1)), + GET_MODE_SIZE (GET_MODE (mem_2)))); + +- /* One of the memory accesses must be a mempair operand. +- If it is not the first one, they need to be swapped by the +- peephole. */ +- if (!aarch64_mem_pair_operand (mem_1, GET_MODE (mem_1)) +- && !aarch64_mem_pair_operand (mem_2, GET_MODE (mem_2))) ++ /* The lower memory access must be a mem-pair operand. */ ++ rtx lower_mem = reversed ? mem_2 : mem_1; ++ if (!aarch64_mem_pair_operand (lower_mem, GET_MODE (lower_mem))) + return false; + + if (REG_P (reg_1) && FP_REGNUM_P (REGNO (reg_1))) +diff --git a/gcc/testsuite/gcc.dg/rtl/aarch64/pr111411.c b/gcc/testsuite/gcc.dg/rtl/aarch64/pr111411.c +new file mode 100644 +index 00000000000..ad07e9c6c89 +--- /dev/null ++++ b/gcc/testsuite/gcc.dg/rtl/aarch64/pr111411.c +@@ -0,0 +1,57 @@ ++/* { dg-do compile { target aarch64*-*-* } } */ ++/* { dg-require-effective-target lp64 } */ ++/* { dg-options "-O -fdisable-rtl-postreload -fpeephole2 -fno-schedule-fusion" } */ ++ ++extern int data[]; ++ ++void __RTL (startwith ("ira")) foo (void *ptr) ++{ ++ (function "foo" ++ (param "ptr" ++ (DECL_RTL (reg/v:DI <0> [ ptr ])) ++ (DECL_RTL_INCOMING (reg/v:DI x0 [ ptr ])) ++ ) ;; param "ptr" ++ (insn-chain ++ (block 2 ++ (edge-from entry (flags "FALLTHRU")) ++ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK) ++ (insn 4 (set (reg:DI <0>) (reg:DI x0))) ++ (insn 5 (set (reg:DI <1>) ++ (plus:DI (reg:DI <0>) (const_int 768)))) ++ (insn 6 (set (mem:SI (plus:DI (reg:DI <0>) ++ (const_int 508)) [1 &data+508 S4 A4]) ++ (const_int 0))) ++ (insn 7 (set (mem:SI (plus:DI (reg:DI <1>) ++ (const_int -256)) [1 &data+512 S4 A4]) ++ (const_int 0))) ++ (edge-to exit (flags "FALLTHRU")) ++ ) ;; block 2 ++ ) ;; insn-chain ++ ) ;; function ++} ++ ++void __RTL (startwith ("ira")) bar (void *ptr) ++{ ++ (function "bar" ++ (param "ptr" ++ (DECL_RTL (reg/v:DI <0> [ ptr ])) ++ (DECL_RTL_INCOMING (reg/v:DI x0 [ ptr ])) ++ ) ;; param "ptr" ++ (insn-chain ++ (block 2 ++ (edge-from entry (flags "FALLTHRU")) ++ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK) ++ (insn 4 (set (reg:DI <0>) (reg:DI x0))) ++ (insn 5 (set (reg:DI <1>) ++ (plus:DI (reg:DI <0>) (const_int 768)))) ++ (insn 6 (set (mem:SI (plus:DI (reg:DI <1>) ++ (const_int -256)) [1 &data+512 S4 A4]) ++ (const_int 0))) ++ (insn 7 (set (mem:SI (plus:DI (reg:DI <0>) ++ (const_int 508)) [1 &data+508 S4 A4]) ++ (const_int 0))) ++ (edge-to exit (flags "FALLTHRU")) ++ ) ;; block 2 ++ ) ;; insn-chain ++ ) ;; function ++}
* see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111418 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111411 Signed-off-by: Martin Jansa <martin.jansa@gmail.com> --- meta/recipes-devtools/gcc/gcc-12.3.inc | 1 + ...ch64-Fix-loose-ldpstp-check-PR111411.patch | 117 ++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 meta/recipes-devtools/gcc/gcc/0026-aarch64-Fix-loose-ldpstp-check-PR111411.patch