Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: "Jan Beulich" <jbeulich@suse.com>,
	"Jürgen Groß" <jgross@suse.com>, "Julien Grall" <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	KonradWilk <konrad.wilk@oracle.com>,
	Ilja Van Sprundel <ivansprundel@ioactive.com>,
	GeorgeDunlap <George.Dunlap@citrix.com>,
	Ian Jackson <ian.jackson@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH] console: avoid buffer overflow in guest_console_write()
Date: Fri, 29 Nov 2019 13:57:03 +0000	[thread overview]
Message-ID: <91765e08-cb02-0d3e-1989-13118c2521e1@citrix.com> (raw)
In-Reply-To: <abddefb1-6fac-67d9-c825-43a1b63300c1@suse.com>

On 29/11/2019 13:55, Jan Beulich wrote:
> On 29.11.2019 14:37, Jürgen Groß wrote:
>> On 29.11.19 14:26, Jan Beulich wrote:
>>> On 29.11.2019 13:37, Andrew Cooper wrote:
>>>> On 29/11/2019 12:19, Jan Beulich wrote:
>>>>> On 29.11.2019 13:15, Andrew Cooper wrote:
>>>>>> On 29/11/2019 12:13, Jan Beulich wrote:
>>>>>>> On 29.11.2019 13:01, Ian Jackson wrote:
>>>>>>>> Jan Beulich writes ("Re: [PATCH] console: avoid buffer overflow in guest_console_write()"):
>>>>>>>>> On 29.11.2019 11:22, Andrew Cooper wrote:
>>>>>>>>>> Is sizeof(array[0]) always 0, or is this just a GCC-ism ?  Godbolt
>>>>>>>>>> suggests is 0 on all compiler we support.
>>>>>>>>>>
>>>>>>>>>> Either way, isn't the more common idiom + 0ul ?  Personally, I feel that
>>>>>>>>>> is clearer to follow.
>>>>>>>>> I decided against + 0ul or alike because in principle size_t
>>>>>>>>> and unsigned long are different types. In particular 32-bit
>>>>>>>>> x86 gcc uses unsigned int for size_t, and hence min()'s
>>>>>>>>> type safety check would cause the build to fail there. The
>>>>>>>>> same risk obviously exists for any 32-bit arch (e.g. Arm32,
>>>>>>>>> but I haven't checked what type it actually uses).
>>>>>>>> I don't know what i wrong with
>>>>>>>>     (size_t)0
>>>>>>>> which is shorter, even !
>>>>>>> True. Yet it contains a cast, no matter how risk-free it may be
>>>>>>> in this case. With a cast, I could as well have written (yet
>>>>>>> shorter) (size_t)count.
>>>>>> Given that min() has a very strict typecheck, I think we should permit
>>>>>> any use of an explicit cast in a single operand, because it *is* safer
>>>>>> than switching to the min_t() route to make things compile.
>>>>> Well, I can switch to (size_t)count if this is liked better
>>>>> overall.
>>>> Personally, I'd prefer this option most of all.
>>> Okay, I've switched to this, but while doing so I started wondering
>>> why we'd then not use
>>>
>>>          kcount = min(count, (unsigned int)sizeof(kbuf) - 1);
>>>
>>> which is an (often slightly cheaper) 32-bit operation (and which
>>> is what I had actually started from).
>> While modifying guest_console_write(), would you mind writing a '\0'
>> to kbuf[kcount]? There is a "conring_puts(kbuf);" later in this
>> function which would like a 0 terminated string as input.
> That's not the right change for this problem, I think. Now that
> we support embedded nul characters, a count should be passed
> instead. Julien?
>
> I also wouldn't want to merge this into this patch; I'm happy to
> send a separate one.

I agree.  Lets fix the concrete stack corruption issue separately from
the concern over NUL-correctness (which was the purpose of the patch
which introduced this vulnerability to start with).

~Andrew

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