[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 10:48 AM, Andrew Cooper wrote:
On 12/02/15 15:38, Boris Ostrovsky wrote:
On 02/12/2015 10:21 AM, Andrew Cooper wrote:
On 12/02/15 15:01, Boris Ostrovsky wrote:
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.
You are correct. My apologies. I however dislike the splitting of the
switch into two.
(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)"?
Better, but overriding the number with a string does hide information.
In the case that the pointer is invalid, it would be useful to see its
contents.
How about "<0xXXXXXX>" (i.e. effectively replace "%pv" with "<%p>",
with angle brackets indicating invalid pointer)?
It feels like change for change sake, especially as there is a perfectly
good hex decode for plain %p.
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
if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
{
switch ( fmt[1] )
{
....
}
}
This makes the patch a whole 3 line addition and indenting the whole
switch block by 4 spaces.
Still don't understand. This will never print anything unless it's a
bad pointer, won't it?
(And if you meant '>=' then we will simply print the invalid pointer
in plain %p format. Which, btw, may be the solution but we will still
need to bump fmt_ptr, so we again will need another switch or
something to test for sub-specifier)
Oops - I did mean >=. I.e. only do the custom %pX decoding in the case
that arg is a plausible pointer.
There is no need I can see to alter the fmt_ptr handling. The code
currently works, other than the issue at hand of falling over a bad pointer.
We do, otherwise we will be printing the sub-specifier. Here is example
of what happens if we don't bump it:
struct vcpu *badvcpu = NULL;
printk("badvcpu: %pv current: %pv\n", badvcpu, current);
console:
badvcpu: 0000000000000000v current: d0v0
Also, for %*ph format, if we just go with falling through to plain
format and not marking somehow that we are printing a bad pointer:
unsigned badval = 0xab;
unsigned *badptr = &badval;
printk("badptr = %*ph\n", 1, badptr);
console:
badptr = ab
We don't know here whether badptr was pointing to 0xab or it itself was
0xab.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
