[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

(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)"?


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


~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);


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.