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

[Stewart Smith]

Balbir Singh <bsingharora at gmail.com> writes:
> On 09/08/16 17:17, Stewart Smith wrote:
>> 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.
>> 
> I'll fix this
>
>>> 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?
>
>
> I'll add a reference to the ISA, BTW in the kernel we assume 60 bits
> and I think it should be fine. I think checking for top 4 bits and
> size of ram should be sufficient. Does doing the implementation
> specific check gain us any more?

I don't think so... I guess we could end up erroring out rather than
checkstopping?

>> 
>>> +		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?
>> 
>
> Good idea, I would like do that incrementally, I'm trying to
> understand how to integrate the error with FWTS, so that the error log
> makes sense.

I'm pretty sure that error logs making sense is rather non-traditional :)

-- 
Stewart Smith
OPAL Architect, IBM.