diff mbox series

[v4,27/53] semihosting: Split out semihost_sys_open

Message ID 20220607204557.658541-28-richard.henderson@linaro.org
State Superseded
Headers show
Series semihosting cleanup | expand

Commit Message

Richard Henderson June 7, 2022, 8:45 p.m. UTC
Split out the non-ARM specific portions of SYS_OPEN to a
reusable function.  This handles gdb and host file i/o.

Add helpers to validate the length of the filename string.
Prepare for usage by other semihosting by allowing the
filename length parameter to be 0, and calling strlen.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/semihosting/syscalls.h |  25 ++++++
 semihosting/arm-compat-semi.c  |  52 ++---------
 semihosting/guestfd.c          |   5 ++
 semihosting/syscalls.c         | 156 +++++++++++++++++++++++++++++++++
 semihosting/meson.build        |   1 +
 5 files changed, 193 insertions(+), 46 deletions(-)
 create mode 100644 include/semihosting/syscalls.h
 create mode 100644 semihosting/syscalls.c

Comments

Luc Michel June 22, 2022, 9:35 a.m. UTC | #1
On 13:45 Tue 07 Jun     , Richard Henderson wrote:
> Split out the non-ARM specific portions of SYS_OPEN to a
> reusable function.  This handles gdb and host file i/o.
> 
> Add helpers to validate the length of the filename string.
> Prepare for usage by other semihosting by allowing the
> filename length parameter to be 0, and calling strlen.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/semihosting/syscalls.h |  25 ++++++
>  semihosting/arm-compat-semi.c  |  52 ++---------
>  semihosting/guestfd.c          |   5 ++
>  semihosting/syscalls.c         | 156 +++++++++++++++++++++++++++++++++
>  semihosting/meson.build        |   1 +
>  5 files changed, 193 insertions(+), 46 deletions(-)
>  create mode 100644 include/semihosting/syscalls.h
>  create mode 100644 semihosting/syscalls.c
> 

[...]

>          } else {
> -            hostfd = open(s, open_modeflags[arg1], 0644);
> -            if (hostfd < 0) {
> -                ret = -1;
> -                err = errno;
> -            } else {
> -                ret = alloc_guestfd();
> -                associate_guestfd(ret, hostfd);
> -            }
> +            semihost_sys_open(cs, common_semi_cb, arg0, arg2 + 1,
> +                              gdb_open_modeflags[arg1], 0644);

You're missing a unlock_user(s, arg0, 0); here
Richard Henderson June 22, 2022, 3:05 p.m. UTC | #2
On 6/22/22 02:35, Luc Michel wrote:
> On 13:45 Tue 07 Jun     , Richard Henderson wrote:
>> Split out the non-ARM specific portions of SYS_OPEN to a
>> reusable function.  This handles gdb and host file i/o.
>>
>> Add helpers to validate the length of the filename string.
>> Prepare for usage by other semihosting by allowing the
>> filename length parameter to be 0, and calling strlen.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   include/semihosting/syscalls.h |  25 ++++++
>>   semihosting/arm-compat-semi.c  |  52 ++---------
>>   semihosting/guestfd.c          |   5 ++
>>   semihosting/syscalls.c         | 156 +++++++++++++++++++++++++++++++++
>>   semihosting/meson.build        |   1 +
>>   5 files changed, 193 insertions(+), 46 deletions(-)
>>   create mode 100644 include/semihosting/syscalls.h
>>   create mode 100644 semihosting/syscalls.c
>>
> 
> [...]
> 
>>           } else {
>> -            hostfd = open(s, open_modeflags[arg1], 0644);
>> -            if (hostfd < 0) {
>> -                ret = -1;
>> -                err = errno;
>> -            } else {
>> -                ret = alloc_guestfd();
>> -                associate_guestfd(ret, hostfd);
>> -            }
>> +            semihost_sys_open(cs, common_semi_cb, arg0, arg2 + 1,
>> +                              gdb_open_modeflags[arg1], 0644);
> 
> You're missing a unlock_user(s, arg0, 0); here

Good catch.  Fixed.


r~
Luc Michel June 27, 2022, 9:22 a.m. UTC | #3
On 08:05 Wed 22 Jun     , Richard Henderson wrote:
> On 6/22/22 02:35, Luc Michel wrote:
> > On 13:45 Tue 07 Jun     , Richard Henderson wrote:
> > > Split out the non-ARM specific portions of SYS_OPEN to a
> > > reusable function.  This handles gdb and host file i/o.
> > > 
> > > Add helpers to validate the length of the filename string.
> > > Prepare for usage by other semihosting by allowing the
> > > filename length parameter to be 0, and calling strlen.
> > > 
> > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > > ---
> > >   include/semihosting/syscalls.h |  25 ++++++
> > >   semihosting/arm-compat-semi.c  |  52 ++---------
> > >   semihosting/guestfd.c          |   5 ++
> > >   semihosting/syscalls.c         | 156 +++++++++++++++++++++++++++++++++
> > >   semihosting/meson.build        |   1 +
> > >   5 files changed, 193 insertions(+), 46 deletions(-)
> > >   create mode 100644 include/semihosting/syscalls.h
> > >   create mode 100644 semihosting/syscalls.c
> > > 
> > 
> > [...]
> > 
> > >           } else {
> > > -            hostfd = open(s, open_modeflags[arg1], 0644);
> > > -            if (hostfd < 0) {
> > > -                ret = -1;
> > > -                err = errno;
> > > -            } else {
> > > -                ret = alloc_guestfd();
> > > -                associate_guestfd(ret, hostfd);
> > > -            }
> > > +            semihost_sys_open(cs, common_semi_cb, arg0, arg2 + 1,
> > > +                              gdb_open_modeflags[arg1], 0644);
> > 
> > You're missing a unlock_user(s, arg0, 0); here
> 
> Good catch.  Fixed.

With this fixed:

Reviewed-by: Luc Michel <lmichel@kalray.eu>


> 
> 
> r~
> 
> 
> To declare a filtering error, please use the following link : https://www.security-mail.net/reporter.php?mid=3812.62b32fac.79d6c.0&r=lmichel%40kalray.eu&s=richard.henderson%40linaro.org&o=Re%3A+%5BPATCH+v4+27%2F53%5D+semihosting%3A+Split+out+semihost_sys_open&verdict=C&c=588dc0ed7d12e87bd5846dca36ad4d902888f28e
> 

--
diff mbox series

Patch

diff --git a/include/semihosting/syscalls.h b/include/semihosting/syscalls.h
new file mode 100644
index 0000000000..991658bf79
--- /dev/null
+++ b/include/semihosting/syscalls.h
@@ -0,0 +1,25 @@ 
+/*
+ * Syscall implementations for semihosting.
+ *
+ * Copyright (c) 2022 Linaro
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef SEMIHOSTING_SYSCALLS_H
+#define SEMIHOSTING_SYSCALLS_H
+
+/*
+ * Argument loading from the guest is performed by the caller;
+ * results are returned via the 'complete' callback.
+ *
+ * String operands are in address/len pairs.  The len argument may be 0
+ * (when the semihosting abi does not already provide the length),
+ * or non-zero (where it should include the terminating zero).
+ */
+
+void semihost_sys_open(CPUState *cs, gdb_syscall_complete_cb complete,
+                       target_ulong fname, target_ulong fname_len,
+                       int gdb_flags, int mode);
+
+#endif /* SEMIHOSTING_SYSCALLS_H */
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 72a1350512..3239eda513 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -32,12 +32,13 @@ 
  */
 
 #include "qemu/osdep.h"
+#include "qemu/timer.h"
+#include "exec/gdbstub.h"
 #include "semihosting/semihost.h"
 #include "semihosting/console.h"
 #include "semihosting/common-semi.h"
 #include "semihosting/guestfd.h"
-#include "qemu/timer.h"
-#include "exec/gdbstub.h"
+#include "semihosting/syscalls.h"
 
 #ifdef CONFIG_USER_ONLY
 #include "qemu.h"
@@ -98,21 +99,6 @@  static int gdb_open_modeflags[12] = {
     GDB_O_RDWR | GDB_O_CREAT | GDB_O_APPEND,
 };
 
-static int open_modeflags[12] = {
-    O_RDONLY,
-    O_RDONLY | O_BINARY,
-    O_RDWR,
-    O_RDWR | O_BINARY,
-    O_WRONLY | O_CREAT | O_TRUNC,
-    O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
-    O_RDWR | O_CREAT | O_TRUNC,
-    O_RDWR | O_CREAT | O_TRUNC | O_BINARY,
-    O_WRONLY | O_CREAT | O_APPEND,
-    O_WRONLY | O_CREAT | O_APPEND | O_BINARY,
-    O_RDWR | O_CREAT | O_APPEND,
-    O_RDWR | O_CREAT | O_APPEND | O_BINARY
-};
-
 #ifndef CONFIG_USER_ONLY
 
 /**
@@ -284,20 +270,6 @@  common_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
     common_semi_cb(cs, ret, err);
 }
 
-static int common_semi_open_guestfd;
-
-static void
-common_semi_open_cb(CPUState *cs, target_ulong ret, target_ulong err)
-{
-    if (err) {
-        dealloc_guestfd(common_semi_open_guestfd);
-    } else {
-        associate_guestfd(common_semi_open_guestfd, ret);
-        ret = common_semi_open_guestfd;
-    }
-    common_semi_cb(cs, ret, err);
-}
-
 /*
  * Types for functions implementing various semihosting calls
  * for specific types of guest file descriptor. These must all
@@ -601,22 +573,10 @@  void do_common_semihosting(CPUState *cs)
                 staticfile_guestfd(ret, featurefile_data,
                                    sizeof(featurefile_data));
             }
-        } else if (use_gdb_syscalls()) {
-            unlock_user(s, arg0, 0);
-            common_semi_open_guestfd = alloc_guestfd();
-            gdb_do_syscall(common_semi_open_cb,
-                           "open,%s,%x,1a4", arg0, (int)arg2 + 1,
-                           gdb_open_modeflags[arg1]);
-            break;
         } else {
-            hostfd = open(s, open_modeflags[arg1], 0644);
-            if (hostfd < 0) {
-                ret = -1;
-                err = errno;
-            } else {
-                ret = alloc_guestfd();
-                associate_guestfd(ret, hostfd);
-            }
+            semihost_sys_open(cs, common_semi_cb, arg0, arg2 + 1,
+                              gdb_open_modeflags[arg1], 0644);
+            break;
         }
         unlock_user(s, arg0, 0);
         common_semi_cb(cs, ret, err);
diff --git a/semihosting/guestfd.c b/semihosting/guestfd.c
index b6405f5663..7ac2e147a8 100644
--- a/semihosting/guestfd.c
+++ b/semihosting/guestfd.c
@@ -11,6 +11,11 @@ 
 #include "qemu/osdep.h"
 #include "exec/gdbstub.h"
 #include "semihosting/guestfd.h"
+#ifdef CONFIG_USER_ONLY
+#include "qemu.h"
+#else
+#include "semihosting/softmmu-uaccess.h"
+#endif
 
 static GArray *guestfd_array;
 
diff --git a/semihosting/syscalls.c b/semihosting/syscalls.c
new file mode 100644
index 0000000000..9f9d19a59a
--- /dev/null
+++ b/semihosting/syscalls.c
@@ -0,0 +1,156 @@ 
+/*
+ * Syscall implementations for semihosting.
+ *
+ * Copyright (c) 2022 Linaro
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "exec/gdbstub.h"
+#include "semihosting/guestfd.h"
+#include "semihosting/syscalls.h"
+#ifdef CONFIG_USER_ONLY
+#include "qemu.h"
+#else
+#include "semihosting/softmmu-uaccess.h"
+#endif
+
+
+/*
+ * Validate or compute the length of the string (including terminator).
+ */
+static int validate_strlen(CPUState *cs, target_ulong str, target_ulong tlen)
+{
+    CPUArchState *env G_GNUC_UNUSED = cs->env_ptr;
+    char c;
+
+    if (tlen == 0) {
+        ssize_t slen = target_strlen(str);
+
+        if (slen < 0) {
+            return -EFAULT;
+        }
+        if (slen >= INT32_MAX) {
+            return -ENAMETOOLONG;
+        }
+        return slen + 1;
+    }
+    if (tlen > INT32_MAX) {
+        return -ENAMETOOLONG;
+    }
+    if (get_user_u8(c, str + tlen - 1)) {
+        return -EFAULT;
+    }
+    if (c != 0) {
+        return -EINVAL;
+    }
+    return tlen;
+}
+
+static int validate_lock_user_string(char **pstr, CPUState *cs,
+                                     target_ulong tstr, target_ulong tlen)
+{
+    int ret = validate_strlen(cs, tstr, tlen);
+    CPUArchState *env G_GNUC_UNUSED = cs->env_ptr;
+    char *str = NULL;
+
+    if (ret > 0) {
+        str = lock_user(VERIFY_READ, tstr, ret, true);
+        ret = str ? 0 : -EFAULT;
+    }
+    *pstr = str;
+    return ret;
+}
+
+/*
+ * GDB semihosting syscall implementations.
+ */
+
+static gdb_syscall_complete_cb gdb_open_complete;
+
+static void gdb_open_cb(CPUState *cs, target_ulong ret, target_ulong err)
+{
+    if (!err) {
+        int guestfd = alloc_guestfd();
+        associate_guestfd(guestfd, ret);
+        ret = guestfd;
+    }
+    gdb_open_complete(cs, ret, err);
+}
+
+static void gdb_open(CPUState *cs, gdb_syscall_complete_cb complete,
+                     target_ulong fname, target_ulong fname_len,
+                     int gdb_flags, int mode)
+{
+    int len = validate_strlen(cs, fname, fname_len);
+    if (len < 0) {
+        complete(cs, -1, -len);
+        return;
+    }
+
+    gdb_open_complete = complete;
+    gdb_do_syscall(gdb_open_cb, "open,%s,%x,%x",
+                   fname, len, (target_ulong)gdb_flags, (target_ulong)mode);
+}
+
+/*
+ * Host semihosting syscall implementations.
+ */
+
+static void host_open(CPUState *cs, gdb_syscall_complete_cb complete,
+                      target_ulong fname, target_ulong fname_len,
+                      int gdb_flags, int mode)
+{
+    CPUArchState *env G_GNUC_UNUSED = cs->env_ptr;
+    char *p;
+    int ret, host_flags;
+
+    ret = validate_lock_user_string(&p, cs, fname, fname_len);
+    if (ret < 0) {
+        complete(cs, -1, -ret);
+        return;
+    }
+
+    if (gdb_flags & GDB_O_WRONLY) {
+        host_flags = O_WRONLY;
+    } else if (gdb_flags & GDB_O_RDWR) {
+        host_flags = O_RDWR;
+    } else {
+        host_flags = O_RDONLY;
+    }
+    if (gdb_flags & GDB_O_CREAT) {
+        host_flags |= O_CREAT;
+    }
+    if (gdb_flags & GDB_O_TRUNC) {
+        host_flags |= O_TRUNC;
+    }
+    if (gdb_flags & GDB_O_EXCL) {
+        host_flags |= O_EXCL;
+    }
+
+    ret = open(p, host_flags, mode);
+    if (ret < 0) {
+        complete(cs, -1, errno);
+    } else {
+        int guestfd = alloc_guestfd();
+        associate_guestfd(guestfd, ret);
+        complete(cs, guestfd, 0);
+    }
+    unlock_user(p, fname, 0);
+}
+
+/*
+ * Syscall entry points.
+ */
+
+void semihost_sys_open(CPUState *cs, gdb_syscall_complete_cb complete,
+                       target_ulong fname, target_ulong fname_len,
+                       int gdb_flags, int mode)
+{
+    if (use_gdb_syscalls()) {
+        gdb_open(cs, complete, fname, fname_len, gdb_flags, mode);
+    } else {
+        host_open(cs, complete, fname, fname_len, gdb_flags, mode);
+    }
+}
diff --git a/semihosting/meson.build b/semihosting/meson.build
index d2c1c37bfd..8057db5494 100644
--- a/semihosting/meson.build
+++ b/semihosting/meson.build
@@ -1,5 +1,6 @@ 
 specific_ss.add(when: 'CONFIG_SEMIHOSTING', if_true: files(
   'guestfd.c',
+  'syscalls.c',
 ))
 
 specific_ss.add(when: ['CONFIG_SEMIHOSTING', 'CONFIG_SOFTMMU'], if_true: files(