[v2,3/4] semihosting/arm-compat-semi: deref parameter register for SYS_HEAPINFO

Message ID 20210309141727.12522-4-alex.bennee@linaro.org
State New
Headers show
Series
  • semihosting/next (SYS_HEAPINFO fix)
Related show

Commit Message

Alex Bennée March 9, 2021, 2:17 p.m.
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(+)

-- 
2.20.1

Comments

Alex Bennée March 9, 2021, 5:01 p.m. | #1
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

Patch

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;