Re: [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer

From: Julien Grall <julien.grall@arm.com>
To: Jan Beulich <JBeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Tim Deegan <tim@xen.org>, Ian Jackson <Ian.Jackson@eu.citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer
Date: Tue, 9 Apr 2019 11:31:40 +0100	[thread overview]
Message-ID: <e623626e-e07f-fb05-f44b-7b298ea69e09@arm.com> (raw)
In-Reply-To: <5CA467E30200007800224225@prv1-mh.provo.novell.com>

Hi,

On 03/04/2019 08:59, Jan Beulich wrote:
>>>> On 02.04.19 at 19:49, <andrew.cooper3@citrix.com> wrote:
>> On 02/04/2019 17:42, Julien Grall wrote:
>>> diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c
>>> index 97466a12b1..35a47c7229 100644
>>> --- a/xen/arch/arm/early_printk.c
>>> +++ b/xen/arch/arm/early_printk.c
>>> @@ -17,9 +17,10 @@
>>>   void early_putch(char c);
>>>   void early_flush(void);
>>>   
>>> -void early_puts(const char *s)
>>> +void early_puts(const char *s, unsigned int nr)
>>
>> size_t here and elsewhere please, because...
>>
>>> @@ -666,16 +664,16 @@ static bool_t console_locks_busted;
>>>   
>>>   static void __putstr(const char *str)
>>>   {
>>> +    size_t len = strlen(str);
>>> +
>>>       ASSERT(spin_is_locked(&console_lock));
>>>   
>>> -    sercon_puts(str);
>>> -    video_puts(str);
>>> +    sercon_puts(str, len);
>>> +    video_puts(str, len);
>>
>> ... this introduces a truncation bug for 64bit builds.
>>
>> I don't expect a 4G buffer to be passed, but it is not worth introducing
>> the possibility for such a subtle bug in the first place.
> 
> I don't entirely object to what you say, but I also don't think
> running into a truncation situation here would be an actual
> problem: Consider how long it would take to get out such a
> giant string, not to speak of it going to wrap our internal ring
> buffers many times. Looking at it from that angle, truncation
> may actually beneficial here;

Truncation could be done explicitly in *_puts functions if necessary. So I will 
switch to size_t as suggested by Andrew.

> "len" may want to be unsigned int
> as well then. do_console_io()'s "count" is "int" as well (should
> really be "unsigned int" imo, just like "cmd").

I will look at writing a patch to switch do_console_io() to unsigned int.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel