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

[Michael Ellerman <mpe@ellerman.id.au>]

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>