From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid Date: Thu, 12 Feb 2015 10:38:13 -0500 Message-ID: <54DCC8E5.8080305@oracle.com> References: <1423688332-3464-1-git-send-email-boris.ostrovsky@oracle.com> <54DC88D3.5080803@citrix.com> <54DCC060.8040208@oracle.com> <54DCC4E6.6070908@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54DCC4E6.6070908@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , ian.campbell@citrix.com, ian.jackson@eu.citrix.com, jbeulich@suse.com, keir@xen.org, tim@xen.org Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 >>>> --- >>>> 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)? > >> >>> >>> 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) -boris