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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jun 28, 2018 at 11:21:35AM +0100, Andrew Cooper wrote:
> 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).

The file in question is an internal state file which is private to the
library, which is assumed to have a good form.  It is a different story
if arbitrary json is accepted as input into the library.

I wouldn't mind changing the internal format, but that doesn't change
the fact that the file is private and is not supposed to be changed
directly by users.

> 
> What is the current behaviour in staging?

The same. The code hasn't changed since 4.7.4.

Wei.

> 
> ~Andrew

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