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

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

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)?


>
>>
>>>
>>> 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)


-boris