Capability to integer casts on CheriBSD

[Richard Kettlewell]

On 29/10/2023 20:36, Julien ÉLIE wrote:
> Hi all,
> 
> I've just tried a build on CheriBSD, using Clang 13.
> It is said to implement memory protection and software 
> compartmentalization features enabled by CHERI-extended CPUs.
> 
> Interestingly, one of the test for checking the validity of a header 
> field body in the test suite core dumps, because of an access to a char 
> before the start of a string.
> It is easily fixed, but what strikes me most is that the error had still 
> not popped up on any other systems, nor was found by static analysers.
> 
> (I bet it would be interesting to run an INN instance for a while on 
> CheriBSD to see it does not core dumps at other places.)
> 
> 
> --- a/lib/headers.c
> +++ b/lib/headers.c
> @@ -47,6 +47,7 @@ bool
>   IsValidHeaderBody(const char *p)
>   {
>       bool emptycontentline = true;
> +    const char *start = p;
> 
>       /* Not NULL and not empty. */
>       if (p == NULL || *p == '\0')
> @@ -73,7 +74,7 @@ IsValidHeaderBody(const char *p)
>                * re-initialize emptycontentline to true. */
>               emptycontentline = true;
>               continue;
> -        } else if (p[-1] == '\r') {
> +        } else if (p > start && p[-1] == '\r') {
>               /* Case of CR not followed by LF (handled at the previous
>                * if statement). */
>               return false;
> 
> 
> 
> 
> That said, the reason I am writing is for two Clang warnings I am unsure 
> they are genuine errors to fix (and also how to fix them if they should).
> 
> 
> In lib/mmap.c, with inn__msync_page(void *p, size_t length, int flags):
> 
> mmap.c:29:33: error: cast from capability type 'void *' to 
> non-capability, non-address type 'size_t' (aka 'unsigned long') is most 
> likely an error [-Werror,-Wcapability-to-integer-cast]
>          char *start = (char *) ((size_t) p & mask);
>                                  ^~~~~~~~~~
> mmap.c:29:23: error: cast from provenance-free integer type to pointer 
> type will give pointer that can not be dereferenced 
> [-Werror,-Wcheri-capability-misuse]
>          char *start = (char *) ((size_t) p & mask);
>                        ^
> mmap.c:30:32: error: cast from capability type 'void *' to 
> non-capability, non-address type 'size_t' (aka 'unsigned long') is most 
> likely an error [-Werror,-Wcapability-to-integer-cast]
>          char *end = (char *) (((size_t) p + length + pagesize) & mask);
>                                 ^~~~~~~~~~
> mmap.c:30:21: error: cast from provenance-free integer type to pointer 
> type will give pointer that can not be dereferenced 
> [-Werror,-Wcheri-capability-misuse]
>          char *end = (char *) (((size_t) p + length + pagesize) & mask);
>                      ^

I don't know much about Cheri but AIUI pointers have extra metadata to 
enforce run-time bounds checking, so synthesizing pointers from integers 
like this does seem rather problematic - there's nowhere for the bound 
information to come from.

The idea of the code in question seems to be to convert region expressed 
by a pointer and length into the slightly wider region containing it 
consisting of whole pages.

An alternative approach that does not synthesize any pointers (but still 
relies on a pointer-to-integer conversion):

   size_t page_mask = pagesize - 1;

   // Offset of p from start of first page
   size_t start_offset = (size_t)p & page_mask;

   // Start of first page
   char *start = p - start_offset;

   // Offset of (p+length) from start of last page, or 0
   // if (p+length) is exactly on a page boundary
   size_t end_offset = (start_offset + length) & page_mask;

   // Offset _backwards_ of (p+length) from end of last page
   if(end_offset > 0)
     end_offset = page_mask - end_offset;

   // Total length of pages
   size_t total_length = start+offset + length + end_offset;

   return msync(start, total_length, flags);

Totally untested - the calculation should be out into a distinct 
function so it can be unit-tested properly.

> And a few other parts of the code, like this one:
> 
> 
> icd.c:490:16: error: cast from capability type 'char *' to 
> non-capability, non-address type 'unsigned long' is most likely an error 
> [-Werror,-Wcapability-to-integer-cast]
>          syslog(L_FATAL, "%s msync failed %s 0x%lx %d %m", LogName, 
> ICDactpath,
>                 (unsigned long) ICDactpointer, ICDactsize);
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

That one should be %p. It would already be broken on a platform with 
32-bit long but 64-bit (or longer) pointers.

ttfn/rjk