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

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

[Andrew Cooper]

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

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

[Boris Ostrovsky]

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>
---
 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. */
-- 
1.7.1

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

[Julien Grall]

Hi Boris,

On 18/02/2015 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>
> ---
>   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).

This assumption is valid on ARM too. Although, we may have some mappings 
after HYPERVISOR_VIRT_START why are not valid.

On ARM, HYPERVISOR_VIRT_END marks the end of mapping which is not always 
mapped (such as the domheap). Would it make sense to test it? Although 
it seems that x86 doesn't have the same meaning for this macro.

Regards,

-- 
Julien Grall

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

[Boris Ostrovsky]

On 02/18/2015 10:52 AM, Julien Grall wrote:
> Hi Boris,
>
> On 18/02/2015 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>
>> ---
>>   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).
>
> This assumption is valid on ARM too. Although, we may have some 
> mappings after HYPERVISOR_VIRT_START why are not valid.
>
> On ARM, HYPERVISOR_VIRT_END marks the end of mapping which is not 
> always mapped (such as the domheap). Would it make sense to test it? 
> Although it seems that x86 doesn't have the same meaning for this macro.


This patch is only trying to avoid obviously bad pointers but it doesn't 
guarantee that a pointer that's not below HYPERVISOR_VIRT_START is good 
(e.g. if you try to print %pv for HYPERVISOR_VIRT_START bad things will 
likely happen).

Since there are cases when we want to dereference something above 
HYPERVISOR_VIRT_END I think adding more tests would make things a bit 
more complicated: we will just keep adding more tests.

I don't know if there is a good solution short of testing "goodness" of 
each pointer (regardless of where it points).

-boris

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

[Jan Beulich]

>>> On 18.02.15 at 16:39, <boris.ostrovsky@oracle.com> wrote:
> --- 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++ = ')';

You should take existing code as reference when adding to it:
Both increments above need to happen unconditionally.

Jan