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

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

On 18/02/15 15:39, 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 value of the
> pointer in parentheses.
>
> For example:
>
>  struct vcpu *v0 = NULL;
>  struct vcpu *v1 = (void *)0xffUL;
>  unsigned val = 0xab;
>  unsigned *ptr = &val;
>  unsigned *badptr = (void *)0xab;
>  printk("v0 = %pv, v1 = %pv, curr = %pv\n", v0, v1, current);
>  printk("badptr = %*ph, ptr = %*ph\n", 1, badptr, 1, ptr);
>
> will produce
>  v0 = (0), v1 = (ff), curr = d0v3
>  badptr = (ab), ptr = ab
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I agree that this looks to be the cleanest solution.

> ---
>  xen/common/vsprintf.c |   23 ++++++++++++++++++++++-
>  1 files changed, 22 insertions(+), 1 deletions(-)
>
> v3:
>  * Print value of the bad pointer in parentheses.
>    (I understand Andrew's dislike of additional switch but I
>    think this is the cleanest way)
>
> 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..5ab61a1 100644
> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -269,7 +269,28 @@ 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 */
> +    /*
> +     * For custom %p suffixes (see XEN_ROOT/docs/misc/printk-formats.txt)
> +     * if arg pointer is bogus then print pointer value in parentheses.
> +     */
> +    if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
> +    {
> +        switch (fmt[1])
> +        {
> +        case 'h':
> +        case 's':
> +        case 'S':
> +        case 'v':
> +            ++*fmt_ptr;
> +            if ( str < end )
> +                *str++ = '(';
> +            str = number(str, end, (unsigned long)arg, 16, -1, -1, ZEROPAD);
> +            if ( str < end )
> +                *str++ = ')';
> +            return str;
> +        }
> +    }
> +
>      switch ( fmt[1] )
>      {
>      case 'h': /* Raw buffer as hex string. */