From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid Date: Thu, 12 Feb 2015 15:21:10 +0000 Message-ID: <54DCC4E6.6070908@citrix.com> References: <1423688332-3464-1-git-send-email-boris.ostrovsky@oracle.com> <54DC88D3.5080803@citrix.com> <54DCC060.8040208@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54DCC060.8040208@oracle.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: Boris Ostrovsky , 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 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. > >> >> 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. ~Andrew