From 09bc0c6be7ead41bff28037af651d3ec915ba436 Mon Sep 17 00:00:00 2001
From: Nathaniel Wesley Filardo <nfilardo@microsoft.com>
Date: Wed, 6 Jul 2022 10:40:35 +0100
Subject: [PATCH] NFC: external_pointer address_cast earlier

Make it easier to justify our avoidance of capptr_from_client and
capptr_reveal in external_pointer by performing address_cast earlier.
In particular, with this change, we can see that the pointer (and so its
authority, in CHERI) is not passed to any called function other than
address_cast and pointer_offset, and so authority is merely propagated
and neither exercised nor amplified.

Remove the long-disused capptr_reveal_wild, which was added for earlier
versions of external_pointer.
---
 src/snmalloc/ds_core/ptrwrap.h | 10 ---------
 src/snmalloc/mem/localalloc.h  | 40 +++++++++++++++++++++-------------
 src/test/func/memory/memory.cc |  2 +-
 3 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/src/snmalloc/ds_core/ptrwrap.h b/src/snmalloc/ds_core/ptrwrap.h
index c1e56f4b6..612adbd7d 100644
--- a/src/snmalloc/ds_core/ptrwrap.h
+++ b/src/snmalloc/ds_core/ptrwrap.h
@@ -423,16 +423,6 @@ namespace snmalloc
     return p.unsafe_ptr();
   }
 
-  /**
-   * Like capptr_reveal, but sometimes we do mean to reveal wild pointers
-   * (specifically in external_pointer, where we're revealing something
-   * architecturally derived from a user pointer).
-   */
-  inline SNMALLOC_FAST_PATH void* capptr_reveal_wild(capptr::AllocWild<void> p)
-  {
-    return p.unsafe_ptr();
-  }
-
   /**
    * Given a void* from the client, it's fine to call it AllocWild.
    * Roughly dual to capptr_reveal().
diff --git a/src/snmalloc/mem/localalloc.h b/src/snmalloc/mem/localalloc.h
index 9ef63b2bd..592625eb5 100644
--- a/src/snmalloc/mem/localalloc.h
+++ b/src/snmalloc/mem/localalloc.h
@@ -750,22 +750,31 @@ namespace snmalloc
     template<Boundary location = Start>
     void* external_pointer(void* p)
     {
-      // Note that each case uses `pointer_offset`, so that on
-      // CHERI it is monotone with respect to the capability.
-      // Note that the returned pointer could be outside the CHERI
-      // bounds of `p`, and thus not something that can be followed.
+      /*
+       * Note that:
+       * * each case uses `pointer_offset`, so that on CHERI, our behaviour is
+       *   monotone with respect to the capability `p`.
+       *
+       * * the returned pointer could be outside the CHERI bounds of `p`, and
+       *   thus not something that can be followed.
+       *
+       * * we don't use capptr_from_client()/capptr_reveal(), to avoid the
+       *   syntactic clutter.  By inspection, `p` flows only to address_cast
+       *   and pointer_offset, and so there's no risk that we follow or act
+       *   to amplify the rights carried by `p`.
+       */
       if constexpr (location == Start)
       {
-        size_t index = index_in_object(p);
+        size_t index = index_in_object(address_cast(p));
         return pointer_offset(p, 0 - index);
       }
       else if constexpr (location == End)
       {
-        return pointer_offset(p, remaining_bytes(p) - 1);
+        return pointer_offset(p, remaining_bytes(address_cast(p)) - 1);
       }
       else
       {
-        return pointer_offset(p, remaining_bytes(p));
+        return pointer_offset(p, remaining_bytes(address_cast(p)));
       }
     }
 
@@ -775,16 +784,17 @@ namespace snmalloc
      * auto p = (char*)malloc(size)
      * remaining_bytes(p + n) == size - n     provided n < size
      */
-    size_t remaining_bytes(const void* p)
+    size_t remaining_bytes(address_t p)
     {
 #ifndef SNMALLOC_PASS_THROUGH
       const PagemapEntry& entry =
-        Config::Backend::template get_metaentry<true>(address_cast(p));
+        Config::Backend::template get_metaentry<true>(p);
 
       auto sizeclass = entry.get_sizeclass();
-      return snmalloc::remaining_bytes(sizeclass, address_cast(p));
+      return snmalloc::remaining_bytes(sizeclass, p);
 #else
-      return pointer_diff(p, reinterpret_cast<void*>(UINTPTR_MAX));
+      return reinterpret_cast<size_t>(
+        std::numeric_limits<decltype(p)>::max() - p);
 #endif
     }
 
@@ -792,7 +802,7 @@ namespace snmalloc
     {
       if (SNMALLOC_LIKELY(Config::is_initialised()))
       {
-        return remaining_bytes(p) >= s;
+        return remaining_bytes(address_cast(p)) >= s;
       }
       return true;
     }
@@ -803,14 +813,14 @@ namespace snmalloc
      * auto p = (char*)malloc(size)
      * index_in_object(p + n) == n     provided n < size
      */
-    size_t index_in_object(const void* p)
+    size_t index_in_object(address_t p)
     {
 #ifndef SNMALLOC_PASS_THROUGH
       const PagemapEntry& entry =
-        Config::Backend::template get_metaentry<true>(address_cast(p));
+        Config::Backend::template get_metaentry<true>(p);
 
       auto sizeclass = entry.get_sizeclass();
-      return snmalloc::index_in_object(sizeclass, address_cast(p));
+      return snmalloc::index_in_object(sizeclass, p);
 #else
       return reinterpret_cast<size_t>(p);
 #endif
diff --git a/src/test/func/memory/memory.cc b/src/test/func/memory/memory.cc
index 292b35f09..2a2ada2ee 100644
--- a/src/test/func/memory/memory.cc
+++ b/src/test/func/memory/memory.cc
@@ -476,7 +476,7 @@ void test_remaining_bytes()
     char* p = (char*)alloc.alloc(size);
     for (size_t offset = 0; offset < size; offset++)
     {
-      auto rem = alloc.remaining_bytes(p + offset);
+      auto rem = alloc.remaining_bytes(address_cast(pointer_offset(p, offset)));
       if (rem != (size - offset))
       {
         printf(