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

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

On 02/12/2015 10:48 AM, Andrew Cooper wrote:
> On 12/02/15 15:38, Boris Ostrovsky wrote:
>> On 02/12/2015 10:21 AM, Andrew Cooper wrote:
>>> On 12/02/15 15:01, Boris Ostrovsky wrote:
>>>> 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.
>>> You are correct.  My apologies.  I however dislike the splitting of the
>>> switch into two.
>>>
>>>> (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)"?
>>> Better, but overriding the number with a string does hide information.
>>> In the case that the pointer is invalid, it would be useful to see its
>>> contents.
>>
>> How about "<0xXXXXXX>" (i.e. effectively replace "%pv" with "<%p>",
>> with angle brackets indicating invalid pointer)?
>>
> It feels like change for change sake, especially as there is a perfectly
> good hex decode for plain %p.
>
>>>>> 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
>>> if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>>> {
>>>       switch ( fmt[1] )
>>>       {
>>>       ....
>>>       }
>>> }
>>>
>>>
>>> This makes the patch a whole 3 line addition and indenting the whole
>>> switch block by 4 spaces.
>> Still don't understand. This will never print anything unless it's a
>> bad pointer, won't it?
>>
>> (And if you meant '>=' then we will simply print the invalid pointer
>> in plain %p format. Which, btw, may be the solution but we will still
>> need to bump fmt_ptr, so we again will need another switch or
>> something to test for sub-specifier)
> Oops - I did mean >=.  I.e. only do the custom %pX decoding in the case
> that arg is a plausible pointer.
>
> There is no need I can see to alter the fmt_ptr handling.  The code
> currently works, other than the issue at hand of falling over a bad pointer.

We do, otherwise we will be printing the sub-specifier. Here is example 
of what happens if we don't bump it:

     struct vcpu *badvcpu = NULL;
     printk("badvcpu: %pv current: %pv\n", badvcpu, current);

console:
     badvcpu: 0000000000000000v current: d0v0


Also, for %*ph format, if we just go with falling through to plain 
format and not marking somehow that we are printing a bad pointer:

     unsigned badval = 0xab;
     unsigned *badptr = &badval;
     printk("badptr = %*ph\n", 1, badptr);

console:
     badptr = ab

We don't know here whether badptr was pointing to 0xab or it itself was 
0xab.

-boris