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

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

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

"(NULL)" is inappropriate for non-null pointers less than VIRT_START.

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.

~Andrew

>      {
>      case 'h': /* Raw buffer as hex string. */
> @@ -277,9 +293,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>          const uint8_t *hex_buffer = arg;
>          unsigned int i;
>  
> -        /* Consumed 'h' from the format string. */
> -        ++*fmt_ptr;
> -
>          /* Bound user count from %* to between 0 and 64 bytes. */
>          if ( field_width <= 0 )
>              return str;
> @@ -306,9 +319,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>          unsigned long sym_size, sym_offset;
>          char namebuf[KSYM_NAME_LEN+1];
>  
> -        /* Advance parents fmt string, as we have consumed 's' or 'S' */
> -        ++*fmt_ptr;
> -
>          s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf);
>  
>          /* If the symbol is not found, fall back to printing the address */
> @@ -335,7 +345,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>      {
>          const struct vcpu *v = arg;
>  
> -        ++*fmt_ptr;
>          if ( str < end )
>              *str = 'd';
>          str = number(str + 1, end, v->domain->domain_id, 10, -1, -1, 0);