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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/02/15 16:33, Boris Ostrovsky wrote:
> 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@xxxxxxxxxx>
>>>>>>> ---
>>>>>>>     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
>

Ah - I see what you mean now.

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

Ok.  As this is only for making debug code less fragile, it probably is
better to explicitly call out a bad pointer differently.

~Andrew


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel