Re: [Xen-devel] [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/06/18 11:17, Wei Liu wrote:
> On Thu, Jun 28, 2018 at 10:27:33AM +0800, Robin Lee wrote:
>> On Wed, Jun 27, 2018 at 8:29 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
>>> On Wed, Jun 27, 2018 at 07:37:42PM +0800, Robin Lee wrote:
>>>> On Wed, Jun 27, 2018 at 7:24 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
>>>>> On Wed, Jun 27, 2018 at 07:08:02PM +0800, Robin Lee wrote:
>>>>>> On Wed, Jun 27, 2018 at 6:58 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
>>>>>>> On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
>>>>>>>> On XenServer 7.1.1, we start a vm with XAPI but attach a block device 
>>>>>>>> with xl.
>>>>>>>> We create an empty json config for the vm with the content "{}\n" and 
>>>>>>>> then
>>>>>>>> run 'xl block-attach':
>>>>>>>>
>>>>>>>>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
>>>>>>>>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse 
>>>>>>>> error: trailing garbage
>>>>>>>>                                       {} K]
>>>>>>>>                        (right here) ------^
>>>>>>>>
>>>>>>>>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to 
>>>>>>>> generate libxl__json_object from JSON representation of 
>>>>>>>> libxl_domain_config.
>>>>>>>>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add 
>>>>>>>> device
>>>>>>>>   libxl_device_disk_add failed.
>>>>>>>>
>>>>>>>> After investigation, we found the buffer returned from 
>>>>>>>> libxl_read_file_contents
>>>>>>>> is not null-terminated. But later in libxl__object_from_json, the 
>>>>>>>> buffer is expected to
>>>>>>>> be null-terminated. So parsing may exceeded the end of file and get in 
>>>>>>>> to uninisialized
>>>>>>>> momery area.
>>>>>>>>
>>>>>>>> Signed-off-by: Robin Lee <robinlee.sysu@xxxxxxxxx>
>>>>>>> I can't seem to be able to reproduce this in upstream xen. Which version
>>>>>>> of Xen does XenServer 7.1.1 have? You can get that from the output of
>>>>>>> `xl info` -- look for xen_{major, minor, extra}.
>>>>>> I also met a strange case. We didn't see this problem with Xen 4.7.1
>>>>>> that released with
>>>>>> XenServer 7.1.1. But since a recently hotfix from XenServer that 
>>>>>> upgraded Xen to
>>>>>> 4.7.4, this problem then shows up.
>>>>>>
>>>>>> The version of yajl (yajl-2.0.4-4.el7.x86_64)  never changed.
>>>>> As far as I can tell, the stored json file already contains trailing 0,
>>>>> even in 4.7.4. There is nothing interesting between 4.7.1 and 4.7.4 in
>>>>> that area of code.
>>>> In my situation, the json file is created with external program and 
>>>> contains
>>>> just "{}\n" and not trailing 0.
>>> Alright. In that case please append 0 to the file you created.
>>>
>>> The json files are considered to be internal to libxl.
>> OK. I can conform that json file generated by xl contains a trailing 0.
>> But that seems not a common design to rely on the trailing 0 inside a text 
>> file.
> I know it is confusing but you probably shouldn't treat libxl-json file
> as a text file. :-)
>
> This also means what you previous did worked by chance -- it probably
> worked because there happened to be 0's after the loaded buffer.

I know libxl isn't hardened at all against bad json, but it is
unreasonable for libxl to depend on there being a \0 at the end of a
json file (which is a text file to literally everyone else and all their
tools).

What is the current behaviour in staging?

~Andrew

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