[Skiboot] [PATCH 1/2] Add infrastructure for pointer validation

[Stewart Smith]

Balbir Singh <bsingharora at gmail.com> writes:
> If the kernel called an OPAL API with vmalloc'd address
> or any other address range in real mode, we would hit
> a problem with aliasing. Since the top 4 bits are ignored
> in real mode, pointers from 0xc.. and 0xd.. (and other ranges)
> could collide and lead to hard to solve bugs. This patch
> adds the infrastructure for pointer validation and a simple
> test case for testing the API
>
> Signed-off-by: Balbir Singh <bsingharora at gmail.com>
> ---
>  core/test/Makefile.check |  1 +
>  core/test/run-api-test.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/opal-api.h       | 23 ++++++++++++++++++++++
>  3 files changed, 75 insertions(+)
>  create mode 100644 core/test/run-api-test.c

Finally got a chance to poke at this and think about it.

I like it.

I have a few thoughts that'd be great to address before merging,
although I'm rather keen to merge.

> --- /dev/null
> +++ b/core/test/run-api-test.c
> @@ -0,0 +1,51 @@
> +/* Copyright 2014-2015 IBM Corp.

Should be to 2016.

> diff --git a/include/opal-api.h b/include/opal-api.h
> index c86244b..e717ca3 100644
> --- a/include/opal-api.h
> +++ b/include/opal-api.h
> @@ -213,6 +213,9 @@
>
>  #ifndef __ASSEMBLY__
>
> +#include <stdbool.h>
> +#include <types.h>
> +
>  /* Other enums */
>  enum OpalVendorApiTokens {
>  	OPAL_START_VENDOR_API_RANGE = 1000, OPAL_END_VENDOR_API_RANGE = 1999
> @@ -1043,6 +1046,26 @@ enum {
>  	OPAL_PCI_TCE_KILL_ALL,
>  };
>
> +extern unsigned long top_of_ram;
> +
> +/*
> + * Returns true if the address is valid, false otherwise
> + *
> + * Checks if the passed address belongs to real address space
> + * or 0xc000... kernel address space. It also checks that
> + * addr <= total physical memory
> + */
> +static inline bool opal_addr_valid(const void *addr)
> +{
> +	unsigned long val = (unsigned long)addr;
> +	if ((val >> 60) != 0xc && (val >> 60) != 0x0)

I think it would be good to point to section 5.7 of the ISA as to where
the 60 comes from. We *could* check for a implementation specific value
too, P8 seems to be 50 bits (although other procs are
different). Although I'm not sure if there's any real value to be had
from that?

> +		return false;
> +	val &= ~0xf000000000000000;
> +	if (val > top_of_ram)
> +		return false;
> +	return true;
> +}

I'm wondering how we can hook this up to prlog(PR_ERROR) or something,
and then have FWTS annotation for it so that fwts would pick it up from
the skiboot log... maybe a macro around it?

-- 
Stewart Smith
OPAL Architect, IBM.