Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 02/12/2015 06:04 AM, Andrew Cooper wrote:
> On 11/02/15 20:58, Boris Ostrovsky wrote:
>> If invalid pointer (i.e. something smaller than HYPERVISOR_VIRT_START)
>> is passed for %*ph/%pv/%ps/%pS format specifiers then print "(NULL)"
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   xen/common/vsprintf.c |   23 ++++++++++++++++-------
>>   1 files changed, 16 insertions(+), 7 deletions(-)
>>
>> v2:
>>   * Print "(NULL)" instead of specifier-specific string
>>   * Consider all addresses under HYPERVISOR_VIRT_START as invalid. (I think
>>     this is true for both x86 and ARM but I don't have ARM platform to test).
>>
>>
>> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
>> index 065cc42..b9542b5 100644
>> --- a/xen/common/vsprintf.c
>> +++ b/xen/common/vsprintf.c
>> @@ -270,6 +270,22 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>>       const char *fmt = *fmt_ptr, *s;
>>   
>>       /* Custom %p suffixes. See XEN_ROOT/docs/misc/printk-formats.txt */
>> +
>> +    switch ( fmt[1] )
>> +    {
>> +        case 'h':
>> +        case 's':
>> +        case 'S':
>> +        case 'v':
>> +            ++*fmt_ptr;
>> +    }
>> +
>> +    if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>> +    {
>> +        char *s = "(NULL)";
>> +        return string(str, end, s, -1, -1, 0);
>> +    }
>> +
>>       switch ( fmt[1] )
> This wont function, as you have inverted the increment of *fmt_ptr and
> check of fmt[1].

fmt value doesn't change, it is stashed at the top of the routine.

(What *is* wrong in the above code is the fact that the arg test is done 
outside the switch. It should be part of the four case statements, 
otherwise we will print plain %p arguments as "NULL").


>
> "(NULL)" is inappropriate for non-null pointers less than VIRT_START.

Yes, I thought about it after I sent it. "(invalid)"?

>
> Given the VIRT check, I would just put the entire switch statement
> inside an "if ( (unsigned long)arg < HYPERVISOR_VIRT_START )" block and
> let it fall through to the plain number case for a bogus pointer.

Not sure I understand what you are suggesting here, sorry.

-boris

>
> ~Andrew
>
>>       {
>>       case 'h': /* Raw buffer as hex string. */
>> @@ -277,9 +293,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>>           const uint8_t *hex_buffer = arg;
>>           unsigned int i;
>>   
>> -        /* Consumed 'h' from the format string. */
>> -        ++*fmt_ptr;
>> -
>>           /* Bound user count from %* to between 0 and 64 bytes. */
>>           if ( field_width <= 0 )
>>               return str;
>> @@ -306,9 +319,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>>           unsigned long sym_size, sym_offset;
>>           char namebuf[KSYM_NAME_LEN+1];
>>   
>> -        /* Advance parents fmt string, as we have consumed 's' or 'S' */
>> -        ++*fmt_ptr;
>> -
>>           s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf);
>>   
>>           /* If the symbol is not found, fall back to printing the address */
>> @@ -335,7 +345,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>>       {
>>           const struct vcpu *v = arg;
>>   
>> -        ++*fmt_ptr;
>>           if ( str < end )
>>               *str = 'd';
>>           str = number(str + 1, end, v->domain->domain_id, 10, -1, -1, 0);