Re: [PATCH] powerpc/pseries: Enforce hcall result buffer validity and size

[Nathan Lynch <nathanl@linux.ibm.com>]

Nathan Lynch <nathanl@linux.ibm.com> writes:
> Michael Ellerman <mpe@ellerman.id.au> writes:
>> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org>
>> writes:
>>>
>>> plpar_hcall(), plpar_hcall9(), and related functions expect callers to
>>> provide valid result buffers of certain minimum size. Currently this
>>> is communicated only through comments in the code and the compiler has
>>> no idea.
>>>
>>> For example, if I write a bug like this:
>>>
>>>   long retbuf[PLPAR_HCALL_BUFSIZE]; // should be PLPAR_HCALL9_BUFSIZE
>>>   plpar_hcall9(H_ALLOCATE_VAS_WINDOW, retbuf, ...);
>>>
>>> This compiles with no diagnostics emitted, but likely results in stack
>>> corruption at runtime when plpar_hcall9() stores results past the end
>>> of the array. (To be clear this is a contrived example and I have not
>>> found a real instance yet.)
>>
>> We did have some real stack corruption bugs in the past.
>>
>> I referred to them in my previous (much uglier) attempt at a fix:
>>
>>   https://patchwork.ozlabs.org/project/linuxppc-dev/patch/1476780032-21643-2-git-send-email-mpe@ellerman.id.au/
>>
>> Annoyingly I didn't describe them in any detail, but at least one of them was:
>>
>>   24c65bc7037e ("hwrng: pseries - port to new read API and fix stack
>>   corruption")
>
> Thanks for this background.
>
>
>> Will this catch a case like that? Where the too-small buffer is not
>> declared locally but rather comes into the function as a pointer?
>
> No, unfortunately. But here's a sketch that forces retbuf to be an
> array [...]

I've made some attempts to improve on this, but I think the original
patch as written may be the best we can do without altering existing
call sites or introducing new APIs and types.

FWIW, GCC is capable of warning when a too-small dynamically allocated
buffer is used. I don't think it would have caught the pseries-rng
bug, but it works when the size of the buffer is available e.g.

  #include <stdlib.h>

  long plpar_hcall(long opcode, long rets[static 4], ...);

  void f(void)
  {
      long retbuf_stack_4[4];
      long retbuf_stack_3[3];
      long *retbuf_heap_4 = malloc(4 * sizeof(long));
      long *retbuf_heap_3 = malloc(3 * sizeof(long));

      plpar_hcall(0, retbuf_stack_4);    
      plpar_hcall(0, retbuf_stack_3); // bug
      plpar_hcall(0, retbuf_heap_4);
      plpar_hcall(0, retbuf_heap_3);  // bug
  }

<source>:13:5: warning: 'plpar_hcall' accessing 32 bytes in a region of size 24 [-Wstringop-overflow=]
   13 |     plpar_hcall(0, retbuf_stack_3); // bug
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:13:5: note: referencing argument 2 of type 'long int[4]'
<source>:3:6: note: in a call to function 'plpar_hcall'
    3 | long plpar_hcall(long opcode, long rets[static 4], ...);
      |      ^~~~~~~~~~~
<source>:15:5: warning: 'plpar_hcall' accessing 32 bytes in a region of size 24 [-Wstringop-overflow=]
   15 |     plpar_hcall(0, retbuf_heap_3);  // bug
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:15:5: note: referencing argument 2 of type 'long int[4]'
<source>:3:6: note: in a call to function 'plpar_hcall'
    3 | long plpar_hcall(long opcode, long rets[static 4], ...);
      |      ^~~~~~~~~~~

Compiler Explorer link for anyone interested in experimenting:
https://godbolt.org/z/x9GKMTzdb

It looks like -Wstringop-overflow is disabled in Linux's build for now,
but hopefully that will change in the future.

OK with taking the patch as-is?