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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

~Andrew


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