Michael Ellerman <[email protected]> writes:
> Nathan Lynch via B4 Relay <[email protected]>
> 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/[email protected]/
>
> 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.
