On Wed, 2 Sept 2026 at 05:43, Brian Cain <[email protected]> wrote:
>
> From: Matheus Tavares Bernardino <[email protected]>
>
> The Hexagon semihosting ABI extends the arm-compatible set with
> operations like OPEN, WRITECREG, WRITE0, ISTTY, STAT, FSTAT, FTELL,
> SEEK, FTRUNC, ACCESS, and GETCWD. Implement these trap0 handlers so
> that baremetal programs using the standard Hexagon simulator ABI can
> perform file I/O when running on qemu-system-hexagon.
Hi; Coverity points out an issue in the filename buffer handling
in this code.
> - /* TODO: implement other hexagon-specific semihosting calls */
> + case HEX_SYS_OPEN:
> + {
> + char filename[BUFSIZ];
> + target_ulong physical_filename_addr;
> + unsigned int filemode;
> + int length;
> + 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);
> + common_semi_cb(cs, -1, ENAMETOOLONG);
> + break;
> + }
> +
> + do {
> + hexagon_read_memory(env, physical_filename_addr + i, 1,
> + &filename[i], retaddr);
> + i++;
> + } while (filename[i - 1]);
Coverity didn't notice this one, but I did. We read into a local
buffer from guest memory, so we can't trust that the guest actually
put a NUL byte terminator in. If it doesn't, then we'll happily
keep loading from guest memory and overflow the filename[] array.
This needs a termination check involving sizeof(filename).
> + case HEX_SYS_STAT:
> + case HEX_SYS_FSTAT:
> + {
> + struct stat st_buf;
> + uint8_t *st_bufptr = (uint8_t *)&sys_stat;
> + int rc, err = 0;
> + char filename[BUFSIZ];
> + target_ulong physical_filename_addr;
> + target_ulong statBufferAddr;
> + hexagon_read_memory(env, swi_info, 4, &physical_filename_addr,
> retaddr);
> +
> + if (what_swi == HEX_SYS_STAT) {
> + int i = 0;
> + do {
> + hexagon_read_memory(env, physical_filename_addr + i, 1,
> + &filename[i], retaddr);
> + i++;
> + } while ((i < BUFSIZ) && filename[i - 1]);
This one does have a check on BUFSIZ, but if we terminate the
loop because of that then filename[] won't be NUL terminated
and stat() might blow up when we pass it.
> + rc = stat(filename, &st_buf);
> + err = errno;
> + case HEX_SYS_ACCESS:
> + {
> + char filename[BUFSIZ];
> + uint32_t FileNameAddr;
> + uint32_t BufferMode;
> + int rc;
> +
> + int i = 0;
> +
> + hexagon_read_memory(env, swi_info, 4, &FileNameAddr, retaddr);
> + do {
> + hexagon_read_memory(env, FileNameAddr + i, 1, &filename[i],
> + retaddr);
> + i++;
> + } while ((i < BUFSIZ) && (filename[i - 1]));
> + filename[i] = 0;
This one is wrong in yet a third way: we do check against
BUFSIZ, and we try to add in a possible missing NUL terminator,
but if we stopped the loop because i == BUFSIZ, then we will
write the NUL terminator off the end of the array.
(This is the one Coverity found: CID 1685942.)
Better still, can't we use the lock_user_string() handling that
the common-semihosting code does? That avoids all this need
for tedious manual byte-at-a-time looping and fixed sized buffers
and potential for bugs.
thanks
-- PMM
