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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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

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