From: "Alex Bennée" <alex.bennee@linaro.org> To: Peter Maydell <peter.maydell@linaro.org> Cc: Keith Packard <keithp@keithp.com>, Bug 1915925 <1915925@bugs.launchpad.net>, QEMU Developers <qemu-devel@nongnu.org> Subject: Re: [PATCH v2 3/4] semihosting/arm-compat-semi: deref parameter register for SYS_HEAPINFO Date: Tue, 09 Mar 2021 17:01:28 +0000 [thread overview] Message-ID: <87r1ko8dlj.fsf@linaro.org> (raw) In-Reply-To: <CAFEAcA_zFYAWc=03iSdsj-Sy+MN5-DWih4QKzddZJsrRjrzhOw@mail.gmail.com> Peter Maydell <peter.maydell@linaro.org> writes: > On Tue, 9 Mar 2021 at 14:23, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> As per the spec: >> >> the PARAMETER REGISTER contains the address of a pointer to a >> four-field data block. >> >> So we need to follow the pointer and place the results of SYS_HEAPINFO >> there. >> >> Bug: https://bugs.launchpad.net/bugs/1915925 >> Cc: Bug 1915925 <1915925@bugs.launchpad.net> >> Cc: Keith Packard <keithp@keithp.com> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> semihosting/arm-compat-semi.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c >> index 733eea1e2d..2ac9226d29 100644 >> --- a/semihosting/arm-compat-semi.c >> +++ b/semihosting/arm-compat-semi.c >> @@ -1210,6 +1210,8 @@ target_ulong do_common_semihosting(CPUState *cs) >> retvals[2] = rambase + limit; /* Stack base */ >> retvals[3] = rambase; /* Stack limit. */ >> #endif >> + /* The result array is pointed to by arg0 */ >> + args = arg0; >> >> for (i = 0; i < ARRAY_SIZE(retvals); i++) { >> bool fail; >> -- > > No, 'args' is the argument array. That's not the same thing > as the data block we're writing, and we shouldn't reassign > that variable here. > > What was wrong with the old arm-semi.c code, which just did > appropriate loads and stores here, and worked fine and was > not buggy ? I was just trying avoid repeating too much verbiage. But OK I've reverted to the original code with the new helper: for (i = 0; i < ARRAY_SIZE(retvals); i++) { bool fail; if (is_64bit_semihosting(env)) { fail = put_user_u64(retvals[i], arg0 + i * 8); } else { fail = put_user_u32(retvals[i], arg0 + i * 4); } if (fail) { /* Couldn't write back to argument block */ errno = EFAULT; return set_swi_errno(cs, -1); } } return 0; > > thanks > -- PMM -- Alex Bennée
WARNING: multiple messages have this Message-ID (diff)
From: "Alex Bennée" <1915925@bugs.launchpad.net> To: qemu-devel@nongnu.org Subject: [Bug 1915925] Re: [PATCH v2 3/4] semihosting/arm-compat-semi: deref parameter register for SYS_HEAPINFO Date: Tue, 09 Mar 2021 17:01:28 -0000 [thread overview] Message-ID: <87r1ko8dlj.fsf@linaro.org> (raw) Message-ID: <20210309170128.HHTVta1WUVlSl1eVfX4E1CnCcPOu3dxKYaAlkz5pJa0@z> (raw) In-Reply-To: CAFEAcA_zFYAWc=03iSdsj-Sy+MN5-DWih4QKzddZJsrRjrzhOw@mail.gmail.com Peter Maydell <peter.maydell@linaro.org> writes: > On Tue, 9 Mar 2021 at 14:23, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> As per the spec: >> >> the PARAMETER REGISTER contains the address of a pointer to a >> four-field data block. >> >> So we need to follow the pointer and place the results of SYS_HEAPINFO >> there. >> >> Bug: https://bugs.launchpad.net/bugs/1915925 >> Cc: Bug 1915925 <1915925@bugs.launchpad.net> >> Cc: Keith Packard <keithp@keithp.com> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> semihosting/arm-compat-semi.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c >> index 733eea1e2d..2ac9226d29 100644 >> --- a/semihosting/arm-compat-semi.c >> +++ b/semihosting/arm-compat-semi.c >> @@ -1210,6 +1210,8 @@ target_ulong do_common_semihosting(CPUState *cs) >> retvals[2] = rambase + limit; /* Stack base */ >> retvals[3] = rambase; /* Stack limit. */ >> #endif >> + /* The result array is pointed to by arg0 */ >> + args = arg0; >> >> for (i = 0; i < ARRAY_SIZE(retvals); i++) { >> bool fail; >> -- > > No, 'args' is the argument array. That's not the same thing > as the data block we're writing, and we shouldn't reassign > that variable here. > > What was wrong with the old arm-semi.c code, which just did > appropriate loads and stores here, and worked fine and was > not buggy ? I was just trying avoid repeating too much verbiage. But OK I've reverted to the original code with the new helper: for (i = 0; i < ARRAY_SIZE(retvals); i++) { bool fail; if (is_64bit_semihosting(env)) { fail = put_user_u64(retvals[i], arg0 + i * 8); } else { fail = put_user_u32(retvals[i], arg0 + i * 4); } if (fail) { /* Couldn't write back to argument block */ errno = EFAULT; return set_swi_errno(cs, -1); } } return 0; > > thanks > -- PMM -- Alex Bennée -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1915925 Title: ARM semihosting HEAPINFO results wrote to wrong address Status in QEMU: Confirmed Bug description: This affects latest development branch of QEMU. According to the ARM spec of the HEAPINFO semihosting call: https://developer.arm.com/documentation/100863/0300/Semihosting- operations/SYS-HEAPINFO--0x16-?lang=en > the PARAMETER REGISTER contains the address of a pointer to a four- field data block. However, QEMU treated the PARAMETER REGISTER as pointing to a four- field data block directly. Here is a simple program that can demonstrate this problem: https://github.com/iNvEr7/qemu-learn/tree/newlib-bug/semihosting- newlib This code links with newlib with semihosting mode, which will call the HEAPINFO SVC during crt0 routine. When running in QEMU (make run), it may crash the program either because of invalid write or memory curruption, depending on the compiled program structure. Also refer to my discussion with newlib folks: https://sourceware.org/pipermail/newlib/2021/018260.html To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1915925/+subscriptions
next prev parent reply other threads:[~2021-03-09 19:02 UTC|newest] Thread overview: 149+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-09 14:17 [PATCH v2 0/4] semihosting/next (SYS_HEAPINFO fix) Alex Bennée 2021-03-09 14:17 ` [PATCH v2 1/4] semihosting: move semihosting tests to multiarch Alex Bennée 2021-03-09 14:17 ` [PATCH v2 2/4] semihosting/arm-compat-semi: unify GET/SET_ARG helpers Alex Bennée 2021-03-09 16:33 ` Peter Maydell 2021-03-09 17:02 ` Keith Packard via 2021-03-09 17:24 ` Alex Bennée 2021-03-09 14:17 ` [PATCH v2 3/4] semihosting/arm-compat-semi: deref parameter register for SYS_HEAPINFO Alex Bennée 2021-03-09 14:17 ` [Bug 1915925] " Alex Bennée 2021-03-09 16:35 ` [Bug 1915925] " Peter Maydell 2021-03-09 16:35 ` Peter Maydell 2021-03-09 17:01 ` Alex Bennée [this message] 2021-03-09 17:01 ` [Bug 1915925] " Alex Bennée 2021-03-09 14:17 ` [PATCH v2 4/4] tests/tcg: add HeapInfo checking to semihosting test Alex Bennée 2021-03-09 17:08 ` Keith Packard via -- strict thread matches above, loose matches on Subject: below -- 2021-03-24 14:29 [PULL for 6.0 00/22] various fixes (kernel-doc, semihosting, testing) Alex Bennée 2021-03-24 14:30 ` [PULL 01/22] scripts/kernel-doc: strip QEMU_ from function definitions Alex Bennée 2021-03-24 14:30 ` [PULL 02/22] docs/devel: include the plugin API information from the headers Alex Bennée 2021-03-24 14:30 ` [PULL 03/22] docs/devel: expand style section of memory management Alex Bennée 2021-03-24 14:30 ` [PULL 04/22] tools/virtiofsd: include --socket-group in help Alex Bennée 2021-03-24 14:30 ` [PULL 05/22] semihosting: move semihosting tests to multiarch Alex Bennée 2021-03-24 14:30 ` [PULL 06/22] semihosting/arm-compat-semi: unify GET/SET_ARG helpers Alex Bennée 2021-03-24 14:30 ` [PULL 07/22] semihosting/arm-compat-semi: don't use SET_ARG to report SYS_HEAPINFO Alex Bennée 2021-03-24 14:30 ` [Bug 1915925] " Alex Bennée 2021-03-24 14:30 ` [PULL 08/22] linux-user/riscv: initialise the TaskState heap/stack info Alex Bennée 2021-03-24 14:30 ` [PULL 09/22] tests/tcg: add HeapInfo checking to semihosting test Alex Bennée 2021-03-24 14:30 ` [PULL 10/22] gitlab-ci.yml: Merge the trace-backend testing into other jobs Alex Bennée 2021-03-24 14:30 ` [PULL 11/22] configure: Don't use the __atomic_*_16 functions for testing 128-bit support Alex Bennée 2021-03-24 14:30 ` [PULL 12/22] cirrus.yml: Update the FreeBSD task to version 12.2 Alex Bennée 2021-03-24 14:30 ` [PULL 13/22] utils: Tighter tests for qemu_strtosz Alex Bennée 2021-03-24 14:30 ` [PULL 14/22] utils: Work around mingw strto*l bug with 0x Alex Bennée 2021-03-24 14:30 ` [PULL 15/22] gitlab: extend timeouts for CFI builds Alex Bennée 2021-03-24 14:30 ` [PULL 16/22] qdev: define list of archs with virtio-pci or virtio-ccw Alex Bennée 2021-03-24 14:30 ` [PULL 17/22] m68k: add the virtio devices aliases Alex Bennée 2021-03-24 14:30 ` [PULL 18/22] blockdev: with -drive if=virtio, use generic virtio-blk Alex Bennée 2021-03-24 14:30 ` [PULL 19/22] iotests: Revert "iotests: use -ccw on s390x for 040, 139, and 182" Alex Bennée 2021-03-24 14:30 ` [PULL 20/22] iotests: test m68k with the virt machine Alex Bennée 2021-03-24 14:30 ` [PULL 21/22] iotests: iothreads need ioeventfd Alex Bennée 2021-03-24 14:30 ` [PULL 22/22] gitlab: default to not building the documentation Alex Bennée 2021-03-24 14:57 ` [PULL for 6.0 00/22] various fixes (kernel-doc, semihosting, testing) no-reply 2021-03-24 17:41 ` Peter Maydell 2021-03-23 16:52 [PATCH for 6.0 v2 00/22] fixes for rc1 pre-PR " Alex Bennée 2021-03-23 16:52 ` [PATCH v2 01/22] scripts/kernel-doc: strip QEMU_ from function definitions Alex Bennée 2021-03-23 16:52 ` [PATCH v2 02/22] docs/devel: include the plugin API information from the headers Alex Bennée 2021-03-23 16:52 ` [PATCH v2 03/22] docs/devel: expand style section of memory management Alex Bennée 2021-03-23 16:52 ` [PATCH v2 04/22] tools/virtiofsd: include --socket-group in help Alex Bennée 2021-03-23 16:52 ` [PATCH v2 05/22] semihosting: move semihosting tests to multiarch Alex Bennée 2021-03-23 16:52 ` [PATCH v2 06/22] semihosting/arm-compat-semi: unify GET/SET_ARG helpers Alex Bennée 2021-03-23 16:52 ` [PATCH v2 07/22] semihosting/arm-compat-semi: don't use SET_ARG to report SYS_HEAPINFO Alex Bennée 2021-03-23 16:52 ` [Bug 1915925] " Alex Bennée 2021-03-23 16:52 ` [PATCH v2 08/22] linux-user/riscv: initialise the TaskState heap/stack info Alex Bennée 2021-03-23 21:29 ` Alistair Francis 2021-03-23 16:52 ` [PATCH v2 09/22] tests/tcg: add HeapInfo checking to semihosting test Alex Bennée 2021-03-24 6:11 ` Thomas Huth 2021-03-24 14:09 ` Richard Henderson 2021-03-23 16:52 ` [PATCH v2 10/22] gitlab-ci.yml: Merge the trace-backend testing into other jobs Alex Bennée 2021-03-23 16:52 ` [PATCH v2 11/22] configure: Don't use the __atomic_*_16 functions for testing 128-bit support Alex Bennée 2021-03-23 16:52 ` [PATCH v2 12/22] cirrus.yml: Update the FreeBSD task to version 12.2 Alex Bennée 2021-03-23 16:52 ` [PATCH v2 13/22] utils: Tighter tests for qemu_strtosz Alex Bennée 2021-03-23 16:53 ` [PATCH v2 14/22] utils: Work around mingw strto*l bug with 0x Alex Bennée 2021-03-23 16:53 ` [PATCH v2 15/22] gitlab: extend timeouts for CFI builds Alex Bennée 2021-03-23 16:53 ` [PATCH v2 16/22] qdev: define list of archs with virtio-pci or virtio-ccw Alex Bennée 2021-03-23 16:53 ` [PATCH v2 17/22] m68k: add the virtio devices aliases Alex Bennée 2021-03-23 16:53 ` [PATCH v2 18/22] blockdev: with -drive if=virtio, use generic virtio-blk Alex Bennée 2021-03-23 16:53 ` [PATCH v2 19/22] iotests: Revert "iotests: use -ccw on s390x for 040, 139, and 182" Alex Bennée 2021-03-23 16:53 ` [PATCH v2 20/22] iotests: test m68k with the virt machine Alex Bennée 2021-03-23 16:53 ` [PATCH v2 21/22] iotests: iothreads need ioeventfd Alex Bennée 2021-03-23 16:53 ` [PATCH v2 22/22] gitlab: default to not building the documentation Alex Bennée 2021-03-23 18:21 ` [PATCH for 6.0 v2 00/22] fixes for rc1 pre-PR (kernel-doc, semihosting, testing) no-reply 2021-03-24 13:40 ` Peter Maydell 2021-03-24 14:22 ` Alex Bennée 2021-03-24 15:58 ` Peter Maydell 2021-03-20 13:36 [PATCH for 6.0 v1 00/14] fixes for rc1 " Alex Bennée 2021-03-20 13:36 ` [PATCH v1 01/14] scripts/kernel-doc: strip QEMU_ from function definitions Alex Bennée 2021-03-20 16:04 ` Richard Henderson 2021-03-20 13:36 ` [PATCH v1 02/14] docs/devel: include the plugin API information from the headers Alex Bennée 2021-03-20 13:36 ` [PATCH v1 03/14] docs/devel: expand style section of memory management Alex Bennée 2021-03-20 16:00 ` Richard Henderson 2021-03-20 13:36 ` [PATCH v1 04/14] tools/virtiofsd: include --socket-group in help Alex Bennée 2021-03-22 11:46 ` Stefan Hajnoczi 2021-03-20 13:36 ` [PATCH v1 05/14] semihosting: move semihosting tests to multiarch Alex Bennée 2021-03-20 16:03 ` Richard Henderson 2021-03-20 13:36 ` [PATCH v1 06/14] semihosting/arm-compat-semi: unify GET/SET_ARG helpers Alex Bennée 2021-03-20 13:36 ` [PATCH v1 07/14] semihosting/arm-compat-semi: don't use SET_ARG to report SYS_HEAPINFO Alex Bennée 2021-03-20 13:36 ` [Bug 1915925] " Alex Bennée 2021-03-20 13:37 ` [PATCH v1 08/14] linux-user/riscv: initialise the TaskState heap/stack info Alex Bennée 2021-03-20 16:05 ` Richard Henderson 2021-03-20 13:37 ` [PATCH v1 09/14] tests/tcg: add HeapInfo checking to semihosting test Alex Bennée 2021-03-20 16:11 ` Richard Henderson 2021-03-20 13:37 ` [PATCH v1 10/14] gitlab-ci.yml: Merge the trace-backend testing into other jobs Alex Bennée 2021-03-22 17:03 ` Willian Rampazzo 2021-03-20 13:37 ` [PATCH v1 11/14] configure: Don't use the __atomic_*_16 functions for testing 128-bit support Alex Bennée 2021-03-20 13:37 ` [PATCH v1 12/14] cirrus.yml: Update the FreeBSD task to version 12.2 Alex Bennée 2021-03-20 13:37 ` [PATCH v1 13/14] utils: Tighter tests for qemu_strtosz Alex Bennée 2021-03-20 13:37 ` [PATCH v1 14/14] utils: Work around mingw strto*l bug with 0x Alex Bennée 2021-03-20 13:54 ` [PATCH for 6.0 v1 00/14] fixes for rc1 (kernel-doc, semihosting, testing) no-reply 2021-03-12 10:20 [PATCH v5 0/5] semihosting/next (SYS_HEAPINFO) Alex Bennée 2021-03-12 10:20 ` [PATCH v5 1/5] semihosting: move semihosting tests to multiarch Alex Bennée 2021-03-12 10:20 ` [PATCH v5 2/5] semihosting/arm-compat-semi: unify GET/SET_ARG helpers Alex Bennée 2021-03-12 10:20 ` [PATCH v5 3/5] semihosting/arm-compat-semi: don't use SET_ARG to report SYS_HEAPINFO Alex Bennée 2021-03-12 10:20 ` [Bug 1915925] " Alex Bennée 2021-03-12 10:32 ` Peter Maydell 2021-03-12 10:32 ` [Bug 1915925] " Peter Maydell 2021-03-12 10:20 ` [PATCH v5 4/5] linux-user/riscv: initialise the TaskState heap/stack info Alex Bennée 2021-03-12 10:20 ` [PATCH v5 5/5] tests/tcg: add HeapInfo checking to semihosting test Alex Bennée 2021-03-12 10:35 ` Peter Maydell 2021-03-12 11:23 ` Alex Bennée 2021-03-12 11:27 ` Peter Maydell 2021-03-12 14:08 ` Alex Bennée 2021-03-09 17:21 [PATCH v3 0/4] semihosting/next (SYS_HEAPINFO) Alex Bennée 2021-03-09 17:21 ` [PATCH v3 1/4] semihosting: move semihosting tests to multiarch Alex Bennée 2021-03-09 17:21 ` [PATCH v3 2/4] semihosting/arm-compat-semi: unify GET/SET_ARG helpers Alex Bennée 2021-03-09 17:21 ` [PATCH v3 4/4] tests/tcg: add HeapInfo checking to semihosting test Alex Bennée 2021-03-11 13:32 ` [PATCH v3 0/4] semihosting/next (SYS_HEAPINFO) Peter Maydell 2021-03-11 20:05 ` Alex Bennée 2021-03-05 13:54 [PATCH v1 0/3] semihosting/next (move from hw, heapinfo) Alex Bennée 2021-03-05 13:54 ` [PATCH v1 1/3] semihosting: Move include/hw/semihosting/ -> include/semihosting/ Alex Bennée 2021-03-05 13:54 ` [PATCH v1 2/3] semihosting: Move hw/semihosting/ -> semihosting/ Alex Bennée 2021-03-05 13:54 ` [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO Alex Bennée 2021-03-05 13:54 ` [Bug 1915925] " Alex Bennée 2021-03-05 14:10 ` Peter Maydell 2021-03-05 14:10 ` [Bug 1915925] " Peter Maydell 2021-03-05 20:22 ` Keith Packard via 2021-03-05 20:22 ` [Bug 1915925] " Keith Packard 2021-03-05 22:54 ` Peter Maydell 2021-03-05 22:54 ` [Bug 1915925] " Peter Maydell 2021-03-05 23:54 ` Keith Packard via 2021-03-05 23:54 ` [Bug 1915925] " Keith Packard 2021-03-06 1:27 ` Richard Henderson 2021-03-06 14:07 ` Peter Maydell 2021-03-06 14:07 ` [Bug 1915925] " Peter Maydell 2021-03-06 16:54 ` Keith Packard via 2021-03-06 16:54 ` [Bug 1915925] " Keith Packard 2021-03-08 10:09 ` Peter Maydell 2021-03-08 10:09 ` [Bug 1915925] " Peter Maydell 2021-03-08 13:36 ` Alistair Francis 2021-03-08 17:28 ` Keith Packard via 2021-03-08 17:28 ` [Bug 1915925] " Keith Packard 2021-03-05 20:19 ` Keith Packard via 2021-03-05 20:19 ` [Bug 1915925] " Keith Packard 2021-02-17 12:19 [Bug 1915925] [NEW] ARM semihosting HEAPINFO results wrote to wrong address iNvEr7 2021-02-17 14:50 ` [Bug 1915925] " Peter Maydell 2021-02-17 15:03 ` Peter Maydell 2021-02-17 16:13 ` Philippe Mathieu-Daudé 2021-03-05 13:33 ` Alex Bennée 2021-03-09 17:21 ` [Bug 1915925] [PATCH v3 3/4] semihosting/arm-compat-semi: don't use SET_ARG to report SYS_HEAPINFO Alex Bennée 2021-03-09 17:21 ` Alex Bennée 2021-03-15 12:46 ` [Bug 1915925] Re: ARM semihosting HEAPINFO results wrote to wrong address Alex Bennée 2021-03-25 11:47 ` Alex Bennée 2021-04-30 8:54 ` Thomas Huth
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87r1ko8dlj.fsf@linaro.org \ --to=alex.bennee@linaro.org \ --cc=1915925@bugs.launchpad.net \ --cc=keithp@keithp.com \ --cc=peter.maydell@linaro.org \ --cc=qemu-devel@nongnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).