From: Michael Ellerman <mpe@ellerman.id.au>
To: Nathan Lynch via B4 Relay
<devnull+nathanl.linux.ibm.com@kernel.org>,
Nicholas Piggin <npiggin@gmail.com>,
Christophe Leroy <christophe.leroy@csgroup.eu>,
"Aneesh Kumar K.V" <aneesh.kumar@kernel.org>,
"Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
Cc: Nathan Lynch <nathanl@linux.ibm.com>, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH] powerpc/pseries: Enforce hcall result buffer validity and size
Date: Tue, 09 Apr 2024 18:53:52 +1000 [thread overview]
Message-ID: <874jcac3xb.fsf@mail.lhotse> (raw)
In-Reply-To: <20240408-pseries-hvcall-retbuf-v1-1-ebc73d7253cf@linux.ibm.com>
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>
next prev parent reply other threads:[~2024-04-09 8:54 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-08 14:08 [PATCH] powerpc/pseries: Enforce hcall result buffer validity and size Nathan Lynch via B4 Relay
2024-04-09 8:53 ` Michael Ellerman [this message]
2024-04-09 13:48 ` Nathan Lynch
2024-04-26 21:45 ` Nathan Lynch
2024-04-29 3:20 ` Michael Ellerman
2024-05-03 10:41 ` Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874jcac3xb.fsf@mail.lhotse \
--to=mpe@ellerman.id.au \
--cc=aneesh.kumar@kernel.org \
--cc=christophe.leroy@csgroup.eu \
--cc=devnull+nathanl.linux.ibm.com@kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=nathanl@linux.ibm.com \
--cc=naveen.n.rao@linux.ibm.com \
--cc=npiggin@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).