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

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

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathanl@linux.ibm.com>

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.)

To make this class of error less likely, we can use explicitly-sized
array parameters instead of pointers in the declarations for the hcall
APIs. When compiled with -Warray-bounds[1], the code above now
provokes a diagnostic like this:

error: array argument is too small;
is of size 32, callee requires at least 72 [-Werror,-Warray-bounds]
   60 |                 plpar_hcall9(H_ALLOCATE_VAS_WINDOW, retbuf,
      |                 ^                                   ~~~~~~

[1] Enabled for LLVM builds but not GCC for now. See commit
    0da6e5fd6c37 ("gcc: disable '-Warray-bounds' for gcc-13 too") and
    related changes.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/include/asm/hvcall.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index a41e542ba94d..39cd1ca4ccb9 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -524,7 +524,7 @@ long plpar_hcall_norets_notrace(unsigned long opcode, ...);
  * Used for all but the craziest of phyp interfaces (see plpar_hcall9)
  */
 #define PLPAR_HCALL_BUFSIZE 4
-long plpar_hcall(unsigned long opcode, unsigned long *retbuf, ...);
+long plpar_hcall(unsigned long opcode, unsigned long retbuf[static PLPAR_HCALL_BUFSIZE], ...);
 
 /**
  * plpar_hcall_raw: - Make a hypervisor call without calculating hcall stats
@@ -538,7 +538,7 @@ long plpar_hcall(unsigned long opcode, unsigned long *retbuf, ...);
  * plpar_hcall, but plpar_hcall_raw works in real mode and does not
  * calculate hypervisor call statistics.
  */
-long plpar_hcall_raw(unsigned long opcode, unsigned long *retbuf, ...);
+long plpar_hcall_raw(unsigned long opcode, unsigned long retbuf[static PLPAR_HCALL_BUFSIZE], ...);
 
 /**
  * plpar_hcall9: - Make a pseries hypervisor call with up to 9 return arguments
@@ -549,8 +549,8 @@ long plpar_hcall_raw(unsigned long opcode, unsigned long *retbuf, ...);
  * PLPAR_HCALL9_BUFSIZE to size the return argument buffer.
  */
 #define PLPAR_HCALL9_BUFSIZE 9
-long plpar_hcall9(unsigned long opcode, unsigned long *retbuf, ...);
-long plpar_hcall9_raw(unsigned long opcode, unsigned long *retbuf, ...);
+long plpar_hcall9(unsigned long opcode, unsigned long retbuf[static PLPAR_HCALL9_BUFSIZE], ...);
+long plpar_hcall9_raw(unsigned long opcode, unsigned long retbuf[static PLPAR_HCALL9_BUFSIZE], ...);
 
 /* pseries hcall tracing */
 extern struct static_key hcall_tracepoint_key;

---
base-commit: bfe51886ca544956eb4ff924d1937ac01d0ca9c8
change-id: 20240408-pseries-hvcall-retbuf-c47c4d70d847

Best regards,
-- 
Nathan Lynch <nathanl@linux.ibm.com>

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

[Michael Ellerman]

Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org>
writes:
> From: Nathan Lynch <nathanl@linux.ibm.com>
>
> 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")

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?

> To make this class of error less likely, we can use explicitly-sized
> array parameters instead of pointers in the declarations for the hcall
> APIs. When compiled with -Warray-bounds[1], the code above now
> provokes a diagnostic like this:
>
> error: array argument is too small;
> is of size 32, callee requires at least 72 [-Werror,-Warray-bounds]
>    60 |                 plpar_hcall9(H_ALLOCATE_VAS_WINDOW, retbuf,
>       |                 ^                                   ~~~~~~
>
> [1] Enabled for LLVM builds but not GCC for now. See commit
>     0da6e5fd6c37 ("gcc: disable '-Warray-bounds' for gcc-13 too") and
>     related changes.

clang build coverage is pretty good these days, so I think it's still
worth doing.

cheers

> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index a41e542ba94d..39cd1ca4ccb9 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -524,7 +524,7 @@ long plpar_hcall_norets_notrace(unsigned long opcode, ...);
>   * Used for all but the craziest of phyp interfaces (see plpar_hcall9)
>   */
>  #define PLPAR_HCALL_BUFSIZE 4
> -long plpar_hcall(unsigned long opcode, unsigned long *retbuf, ...);
> +long plpar_hcall(unsigned long opcode, unsigned long retbuf[static PLPAR_HCALL_BUFSIZE], ...);
>  
>  /**
>   * plpar_hcall_raw: - Make a hypervisor call without calculating hcall stats
> @@ -538,7 +538,7 @@ long plpar_hcall(unsigned long opcode, unsigned long *retbuf, ...);
>   * plpar_hcall, but plpar_hcall_raw works in real mode and does not
>   * calculate hypervisor call statistics.
>   */
> -long plpar_hcall_raw(unsigned long opcode, unsigned long *retbuf, ...);
> +long plpar_hcall_raw(unsigned long opcode, unsigned long retbuf[static PLPAR_HCALL_BUFSIZE], ...);
>  
>  /**
>   * plpar_hcall9: - Make a pseries hypervisor call with up to 9 return arguments
> @@ -549,8 +549,8 @@ long plpar_hcall_raw(unsigned long opcode, unsigned long *retbuf, ...);
>   * PLPAR_HCALL9_BUFSIZE to size the return argument buffer.
>   */
>  #define PLPAR_HCALL9_BUFSIZE 9
> -long plpar_hcall9(unsigned long opcode, unsigned long *retbuf, ...);
> -long plpar_hcall9_raw(unsigned long opcode, unsigned long *retbuf, ...);
> +long plpar_hcall9(unsigned long opcode, unsigned long retbuf[static PLPAR_HCALL9_BUFSIZE], ...);
> +long plpar_hcall9_raw(unsigned long opcode, unsigned long retbuf[static PLPAR_HCALL9_BUFSIZE], ...);
>  
>  /* pseries hcall tracing */
>  extern struct static_key hcall_tracepoint_key;
>
> ---
> base-commit: bfe51886ca544956eb4ff924d1937ac01d0ca9c8
> change-id: 20240408-pseries-hvcall-retbuf-c47c4d70d847
>
> Best regards,
> -- 
> Nathan Lynch <nathanl@linux.ibm.com>

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

[Nathan Lynch]

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, along with the necessary changes to make pseries-rng build:

diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index 39cd1ca4ccb9..4055e461dcfd 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -524,7 +524,11 @@ long plpar_hcall_norets_notrace(unsigned long opcode, ...);
  * Used for all but the craziest of phyp interfaces (see plpar_hcall9)
  */
 #define PLPAR_HCALL_BUFSIZE 4
-long plpar_hcall(unsigned long opcode, unsigned long retbuf[static PLPAR_HCALL_BUFSIZE], ...);
+long _plpar_hcall(unsigned long opcode, unsigned long retbuf[static PLPAR_HCALL_BUFSIZE], ...);
+#define plpar_hcall(opcode_, retbuf_, ...) ({				\
+			static_assert(ARRAY_SIZE(retbuf_) >= PLPAR_HCALL_BUFSIZE); \
+			_plpar_hcall(opcode_, retbuf_, ## __VA_ARGS__);	\
+		})
 
 /**
  * plpar_hcall_raw: - Make a hypervisor call without calculating hcall stats
diff --git a/arch/powerpc/platforms/pseries/hvCall.S b/arch/powerpc/platforms/pseries/hvCall.S
index 2b0cac6fb61f..4570dc0648fc 100644
--- a/arch/powerpc/platforms/pseries/hvCall.S
+++ b/arch/powerpc/platforms/pseries/hvCall.S
@@ -147,7 +147,7 @@ plpar_hcall_norets_trace:
 	blr
 #endif
 
-_GLOBAL_TOC(plpar_hcall)
+_GLOBAL_TOC(_plpar_hcall)
 	HMT_MEDIUM
 
 	mfcr	r0
diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index 4e9916bb03d7..11738c40274c 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -54,7 +54,7 @@
 
 
 /* in hvCall.S */
-EXPORT_SYMBOL(plpar_hcall);
+EXPORT_SYMBOL(_plpar_hcall);
 EXPORT_SYMBOL(plpar_hcall9);
 EXPORT_SYMBOL(plpar_hcall_norets);
 
diff --git a/drivers/char/hw_random/pseries-rng.c b/drivers/char/hw_random/pseries-rng.c
index 62bdd5af1339..a8cc6b80cd76 100644
--- a/drivers/char/hw_random/pseries-rng.c
+++ b/drivers/char/hw_random/pseries-rng.c
@@ -15,10 +15,10 @@
 
 static int pseries_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
-	u64 buffer[PLPAR_HCALL_BUFSIZE];
+	unsigned long buffer[PLPAR_HCALL_BUFSIZE];
 	int rc;
 
-	rc = plpar_hcall(H_RANDOM, (unsigned long *)buffer);
+	rc = plpar_hcall(H_RANDOM, buffer);
 	if (rc != H_SUCCESS) {
 		pr_err_ratelimited("H_RANDOM call failed %d\n", rc);
 		return -EIO;


There may be other call sites to fix but this is enough for
ppc64le_defconfig.

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

[Nathan Lynch]

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?

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

[Michael Ellerman]

Nathan Lynch <nathanl@linux.ibm.com> writes:
> 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.
...
>
> OK with taking the patch as-is?

Yeah. It's an improvement, even if it's not the full solution.

cheers

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

[Michael Ellerman]

On Mon, 08 Apr 2024 09:08:31 -0500, Nathan Lynch wrote:
> 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:
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/pseries: Enforce hcall result buffer validity and size
      https://git.kernel.org/powerpc/c/ff2e185cf73df480ec69675936c4ee75a445c3e4

cheers