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.
