From 6b34765d5d0fdc0deb6a35034fa56cbb5165cdce Mon Sep 17 00:00:00 2001
From: Alexander Richardson <alexrichardson@google.com>
Date: Thu, 16 May 2024 10:28:10 -0700
Subject: [PATCH] arch-generic: Avoid out-of-memory errors for bad semihosting
 calls (#1143)

In BaseSemihosting::readString() we were using the len argument to
allocate a std::vector without checking whether the value makes any
sense. This resulted in a std::bad_alloc exception being raised prior to
https://github.com/gem5/gem5/pull/1142 for my semihosting tests. This
commit prevents semihosting from reading more than 64K for string
arguments which should be more than sufficient for any valid code.

Change-Id: I059669016ee2c5721fedb914595d0494f6cfd4cd
---
 src/arch/generic/semihosting.cc | 37 ++++++++++++++++++++++++---------
 src/arch/generic/semihosting.hh |  5 ++++-
 2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/src/arch/generic/semihosting.cc b/src/arch/generic/semihosting.cc
index 00ac7c1576..95ff80ce72 100644
--- a/src/arch/generic/semihosting.cc
+++ b/src/arch/generic/semihosting.cc
@@ -157,9 +157,18 @@ BaseSemihosting::unserialize(CheckpointIn &cp)
         files[i] = FileBase::create(*this, cp, csprintf("file%i", i));
 }
 
-std::string
+std::optional<std::string>
 BaseSemihosting::readString(ThreadContext *tc, Addr ptr, size_t len)
 {
+    if (len > 65536) {
+      // If the semihosting call passes an incorrect argument, reject it rather
+      // than attempting to allocate a buffer of that size. We chose 64K as
+      // an arbitrary limit here since no valid program should be attempting
+      // to open a file with such a large filename.
+      warn("BaseSemihosting::readString(): attempting to read too large "
+           "(%d bytes) string from %#x", len, ptr);
+      return std::nullopt;
+    }
     std::vector<char> buf(len + 1);
 
     buf[len] = '\0';
@@ -179,7 +188,10 @@ BaseSemihosting::callOpen(
     if (!mode || !name_base)
         return retError(EINVAL);
 
-    std::string fname = readString(tc, name_base, name_size);
+    std::optional<std::string> fnameOpt = readString(tc, name_base, name_size);
+    if (!fnameOpt.has_value())
+        return retError(ERANGE);
+    std::string fname = *fnameOpt;
     if (!fname.empty() && fname.front() != '/' && fname != ":tt" &&
             fname != ":semihosting-features")
         fname = filesRootDir + fname;
@@ -364,9 +376,11 @@ BaseSemihosting::RetErrno
 BaseSemihosting::callRemove(
         ThreadContext *tc, Addr name_base, size_t name_size)
 {
-    std::string fname = readString(tc, name_base, name_size);
+    std::optional<std::string> fname = readString(tc, name_base, name_size);
 
-    if (remove(fname.c_str()) != 0) {
+    if (!fname.has_value()) {
+        return retError(ERANGE);
+    } else if (remove(fname->c_str()) != 0) {
         return retError(errno);
     } else {
         return retOK(0);
@@ -377,10 +391,11 @@ BaseSemihosting::RetErrno
 BaseSemihosting::callRename(ThreadContext *tc, Addr from_addr,
         size_t from_size, Addr to_addr, size_t to_size)
 {
-    std::string from = readString(tc, from_addr, from_size);
-    std::string to = readString(tc, to_addr, to_size);
-
-    if (rename(from.c_str(), to.c_str()) != 0) {
+    std::optional<std::string> from = readString(tc, from_addr, from_size);
+    std::optional<std::string> to = readString(tc, to_addr, to_size);
+    if (!from.has_value() || !to.has_value()) {
+        return retError(ERANGE);
+    } else if (rename(from->c_str(), to->c_str()) != 0) {
         return retError(errno);
     } else {
         return retOK(0);
@@ -402,9 +417,11 @@ BaseSemihosting::callTime(ThreadContext *tc)
 BaseSemihosting::RetErrno
 BaseSemihosting::callSystem(ThreadContext *tc, Addr cmd_addr, size_t cmd_size)
 {
-    const std::string cmd = readString(tc, cmd_addr, cmd_size);
+    const std::optional<std::string> cmd = readString(tc, cmd_addr, cmd_size);
+    if (!cmd.has_value())
+      return retError(ERANGE);
     warn("Semihosting: SYS_SYSTEM not implemented. Guest tried to run: %s\n",
-            cmd);
+            *cmd);
     return retError(EINVAL);
 }
 
diff --git a/src/arch/generic/semihosting.hh b/src/arch/generic/semihosting.hh
index 8725e25389..fe571b7af2 100644
--- a/src/arch/generic/semihosting.hh
+++ b/src/arch/generic/semihosting.hh
@@ -42,6 +42,7 @@
 #include <functional>
 #include <map>
 #include <memory>
+#include <optional>
 #include <utility>
 #include <vector>
 
@@ -381,7 +382,9 @@ class BaseSemihosting : public SimObject
         return tick >> tickShift;
     }
     void semiExit(uint64_t code, uint64_t subcode);
-    std::string readString(ThreadContext *tc, Addr ptr, size_t len);
+
+    std::optional<std::string> readString(ThreadContext *tc,
+                                          Addr ptr, size_t len);
 
   public:
     typedef std::pair<uint64_t, SemiErrno> RetErrno;