On Mon, 14 Sept 2026 at 23:32, Brian Cain <[email protected]> wrote:
>
> HEX_SYS_OPEN copied guest bytes into a fixed-size buffer until it found a NUL.
> A missing terminator could overrun that buffer. Use lock_user() with the
> ABI filename length and validate its terminating NUL before opening it.
>
> Fixes: 7711fdba88b ("target/hexagon: add main arch-specific semihosting
> operations")
> Link:
> https://lore.kernel.org/all/cafeaca9mos6vfhf2uhz8z4cdvab+7dy6hjb2emyz2alse-t...@mail.gmail.com/
> Suggested-by: Peter Maydell <[email protected]>
> Signed-off-by: Brian Cain <[email protected]>
> ---
> target/hexagon/hexswi.c | 34 ++++++++++++++++------------------
> 1 file changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/target/hexagon/hexswi.c b/target/hexagon/hexswi.c
> index 4705e915aea..21e5d630a75 100644
> --- a/target/hexagon/hexswi.c
> +++ b/target/hexagon/hexswi.c
> @@ -28,6 +28,7 @@
> #include "semihosting/console.h"
> #include "semihosting/syscalls.h"
> #include "semihosting/guestfd.h"
> +#include "semihosting/uaccess.h"
> #include "system/runstate.h"
>
> /* non-arm-compatible semihosting calls */
> @@ -461,31 +462,17 @@ static void sim_handle_trap0(CPUHexagonState *env)
>
> case HEX_SYS_OPEN:
> {
> - char filename[BUFSIZ];
> + char *filename;
> target_ulong physical_filename_addr;
> unsigned int filemode;
> - int length;
> + uint32_t filename_len;
> + size_t filename_size;
> int real_openmode;
> int ret, err = 0;
> - int i = 0;
>
> hexagon_read_memory(env, swi_info, 4, &physical_filename_addr,
> retaddr);
> hexagon_read_memory(env, swi_info + 4, 4, &filemode, retaddr);
> - hexagon_read_memory(env, swi_info + 8, 4, &length, retaddr);
> -
> - if (length >= BUFSIZ) {
> - qemu_log_mask(LOG_GUEST_ERROR,
> - "%s: filename too large (%d)\n",
> - __func__, length);
> - semi_cb(cs, -1, ENAMETOOLONG);
> - break;
> - }
> -
> - do {
> - hexagon_read_memory(env, physical_filename_addr + i, 1,
> - &filename[i], retaddr);
> - i++;
> - } while (filename[i - 1]);
> + hexagon_read_memory(env, swi_info + 8, 4, &filename_len, retaddr);
This looks like an endianness bug -- we read 4 bytes of guest-endianness
length into filename_len and then treat it as a host value without
doing any byteswapping. Could we use get_user_u32() here ? That helper
does the memory load and any necessary byteswapping for you.
(This seems to be a problem with a lot of the code in this file that's
using hexagon_read_memory() to read from guest memory into host
variables. If you made it use the uaccess.h helpers instead you would fix
all those big-endian-host bugs and you might find that you don't need
hexagon_read_memory() at all once you've done that.)
> /* convert ARM ANGEL filemode into host filemode */
> if (filemode < ARRAY_SIZE(angel_to_host_filemode_table)) {
> @@ -498,6 +485,16 @@ static void sim_handle_trap0(CPUHexagonState *env)
> break;
> }
>
> + /* The ABI length excludes the filename's terminating NUL. */
> + filename_size = (size_t)filename_len + 1;
> + filename = lock_user(VERIFY_READ, physical_filename_addr,
> + filename_size, true);
> + if (!filename || filename[filename_len] != '\0') {
> + unlock_user(filename, physical_filename_addr, 0);
> + semi_cb(cs, -1, EFAULT);
> + break;
> + }
> +
This splits the two parts of handling the filename argument so
that the "check and convert the filemode argument" code is
in the middle of it. Could we put all the filename-argument
code in one place?
-- PMM
