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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.04.19 at 19:49, <andrew.cooper3@xxxxxxxxxx> 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; "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've gone through all the existing __putstr() callers: They
all pass either string literals or addresses of static arrays.

Jan



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