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

[Balbir Singh]

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?
> 
>> +		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.

Thanks for the review,
Balbir Singh