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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

~Andrew