[Bug sanitizer/100665] New: [hwsanitizer] nested funtion pointer is tagged but never checked.

[Bug sanitizer/100665] New: [hwsanitizer] nested funtion pointer is tagged but never checked.

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100665

            Bug ID: 100665
           Summary: [hwsanitizer] nested funtion pointer is tagged but
                    never checked.
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: crazylht at gmail dot com
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    hjl.tools at gmail dot com, jakub at gcc dot gnu.org,
                    kcc at gcc dot gnu.org, marxin at gcc dot gnu.org,
                    matmal01 at gcc dot gnu.org
  Target Milestone: ---

testcase is gcc/testsuite/gcc.dg/hwasan/nested-functions-0.c

__attribute__((noinline))
int *Ident(void *x) {
  return x;
}

int __attribute__ ((noinline))
intermediate (void (*f) (int, char),
              char num)
{
  if (num == 1)
    /* NOTE: We need to overrun by an amount greater than the "extra data" in a
       nonlocal goto structure.  The entire structure is allocated on the stack
       with a single tag, which means hwasan can't tell if a closed-over buffer
       was overrun by an amount small enough that the access was still to some
       data in that nonlocal goto structure.  */
    f (100, 100);
  else
    f (3, 100);
  /* Just return something ... */
  return num % 3;
}

int* __attribute__ ((noinline))
nested_function (char num)
{
  int big_array[16];
  int other_array[16];
  void store (int index, char value)
    { big_array[index] = value; }
  return Ident(&other_array[intermediate (store, num)]);
}

#ifndef MAIN
int main ()
{
  nested_function (0);
  return 0;
}
#endif


nest function store is defined and resides one the stack of nested_function,
function pointer of store will be tagged since hwasan thought it was stack
variable, but since there's no explicit load for the function pointer, the tag
is never checked, so i wonder, is hwasan supposed to tag the function pointer?

[Bug sanitizer/100665] [hwsanitizer] nested funtion pointer is tagged but never checked.

[matmal01 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100665

--- Comment #1 from Matthew Malcomson <matmal01 at gcc dot gnu.org> ---
Hi there.
I believe this is how it should work (if I'm understanding & remembering
correctly).

When creating a nested function, we make a single object on the stack that
includes all variables used in the nested function plus a trampoline.
This is called the "nonlocal frame struct" as described in gcc/tree-nested.c.

That single object gets a single tag like all other objects in tagged memory
(trying to separate the closed-over objects from the trampoline and argument
pointers would be pretty awkward when the object is just one struct as far as
the expand code is concerned).

That tag is checked when accessing the closed over variables (i.e. big_array in
the example), so we definitely want to tag the object.

Given that, the question of whether the function pointer (i.e. the pointer to
the trampoline inside that object) should be tagged when passed elsewhere then
has a few benefits:
1) In this case there is no check performed, but there may be checks performed
   if e.g. this function pointer gets cast to an integer pointer and some code
   elsewhere attempts to read that integer.
2) This is just more self-consistent.  Every pointer to a tagged object is
   tagged with the same value.
3) There are hardware extensions to automatically check memory accesses.  If
the
   function pointer is not tagged in this case then (at least for AArch64) the
   PC-relative ldr's in the trampoline stored in that structure will end up
   without a tag and I believe that would trigger a fault.

Point (1) is the main one.  In general when passing a pointer into another
function we don't know if it's going to be accessed or not, so we always need
to
pass tagged pointers.

[Bug sanitizer/100665] [hwsanitizer] nested funtion pointer is tagged but never checked.

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100665

--- Comment #2 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Matthew Malcomson from comment #1)
> Hi there.
> I believe this is how it should work (if I'm understanding & remembering
> correctly).
> 
> When creating a nested function, we make a single object on the stack that
> includes all variables used in the nested function plus a trampoline.
> This is called the "nonlocal frame struct" as described in gcc/tree-nested.c.
> 
> That single object gets a single tag like all other objects in tagged memory
> (trying to separate the closed-over objects from the trampoline and argument
> pointers would be pretty awkward when the object is just one struct as far as
> the expand code is concerned).
> 
> That tag is checked when accessing the closed over variables (i.e. big_array
> in
> the example), so we definitely want to tag the object.
> 

This sounds reasonable.

> Given that, the question of whether the function pointer (i.e. the pointer to
> the trampoline inside that object) should be tagged when passed elsewhere
> then
> has a few benefits:
> 1) In this case there is no check performed, but there may be checks
> performed
>    if e.g. this function pointer gets cast to an integer pointer and some
> code
>    elsewhere attempts to read that integer.
I'm not sure there're cases where code pointers are casted to integer pointers.
But consider the above comment, I agree that tag is needed for the object.

BTW, I bring this up because I'm working on supporting hwsan with Intel LAM,
but unlike TBI, Intel LAM only supports tagging of data pointer. So it looks
like we need to mask off code pointers for indirect call or jmp in compiler-rt.

[Bug sanitizer/100665] [hwsanitizer] nested funtion pointer is tagged but never checked.

[matmal01 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100665

Matthew Malcomson <matmal01 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #3 from Matthew Malcomson <matmal01 at gcc dot gnu.org> ---
(In reply to Hongtao.liu from comment #2)
> (In reply to Matthew Malcomson from comment #1)
> > Given that, the question of whether the function pointer (i.e. the pointer to
> > the trampoline inside that object) should be tagged when passed elsewhere
> > then
> > has a few benefits:
> > 1) In this case there is no check performed, but there may be checks
> > performed
> >    if e.g. this function pointer gets cast to an integer pointer and some
> > code
> >    elsewhere attempts to read that integer.
> I'm not sure there're cases where code pointers are casted to integer
> pointers. But consider the above comment, I agree that tag is needed for the
> object.

Fair ;-).
My reasoning was along the lines of "it's an escaped pointer, and I don't know
what other code will do with it" than actually expecting that to happen.