xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] libxl: Fix NULL pointer due to XSA-178 fix wrong XS nodename
@ 2016-06-08 14:56 Ian Jackson
  2016-06-08 15:11 ` Wei Liu
  0 siblings, 1 reply; 3+ messages in thread
From: Ian Jackson @ 2016-06-08 14:56 UTC (permalink / raw)
  To: xen-devel; +Cc: Wei Liu, Ian Jackson, security, Jan Beulich

In "libxl: Do not trust backend for disk eject vdev" (c69871a2fb26 on
xen.git#staging) we changed libxl_evenable_disk_eject to read the
device vdev out of xenstore from the /libxl path, rather than the
backend path, and to read it during setup rather than on each event.

However, the patch has a mistake:
    -        GCSPRINTF("%s/dev", backend), NULL);
    +        GCSPRINTF("%s/vdev", libxl_path), &configured_vdev);
                           ^
Spot the extra "v".  This causes configured_vdev always to be NULL.
configured_vdev is passed to [libxl__]strdup.

In Xen 4.6 and later libxl__strdup is used and tolerates NULL.
evg->vdev is set to NULL.  This propagates to the `vdev' field in the
generated event.  This may or may not cause further trouble, depending
on the calling application.  In our osstest test cases it does not
cause any trouble, so the bug goes undetected.

In Xen 4.5 and earlier, the strdup does not tolerate NULL, and libxl
crashes immediately.  This has been detected by osstest as a
regression in Xen 4.5.

IMO this patch should be applied immediately to
  xen.git#staging-4.5 (to check that it fixes the osstest regression)
  xen.git#staging     (to check that it does not break master

Subject to passes, it should then be propagated to all supported
stable trees and also be mentioned in an update to XSA-178.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: security@xenproject.org
CC: Jan Beulich <jbeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 006b83f..7584966 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1399,7 +1399,7 @@ int libxl_evenable_disk_eject(libxl_ctx *ctx, uint32_t guest_domid,
 
     const char *configured_vdev;
     rc = libxl__xs_read_checked(gc, XBT_NULL,
-            GCSPRINTF("%s/vdev", libxl_path), &configured_vdev);
+            GCSPRINTF("%s/dev", libxl_path), &configured_vdev);
     if (rc) goto out;
 
     evg->vdev = libxl__strdup(NOGC, configured_vdev);
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] libxl: Fix NULL pointer due to XSA-178 fix wrong XS nodename
  2016-06-08 14:56 [PATCH] libxl: Fix NULL pointer due to XSA-178 fix wrong XS nodename Ian Jackson
@ 2016-06-08 15:11 ` Wei Liu
  2016-06-08 15:18   ` Ian Jackson
  0 siblings, 1 reply; 3+ messages in thread
From: Wei Liu @ 2016-06-08 15:11 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel, Wei Liu, security, Jan Beulich

On Wed, Jun 08, 2016 at 03:56:36PM +0100, Ian Jackson wrote:
> In "libxl: Do not trust backend for disk eject vdev" (c69871a2fb26 on
> xen.git#staging) we changed libxl_evenable_disk_eject to read the
> device vdev out of xenstore from the /libxl path, rather than the
> backend path, and to read it during setup rather than on each event.
> 
> However, the patch has a mistake:
>     -        GCSPRINTF("%s/dev", backend), NULL);
>     +        GCSPRINTF("%s/vdev", libxl_path), &configured_vdev);
>                            ^
> Spot the extra "v".  This causes configured_vdev always to be NULL.
> configured_vdev is passed to [libxl__]strdup.
> 
> In Xen 4.6 and later libxl__strdup is used and tolerates NULL.
> evg->vdev is set to NULL.  This propagates to the `vdev' field in the
> generated event.  This may or may not cause further trouble, depending
> on the calling application.  In our osstest test cases it does not
> cause any trouble, so the bug goes undetected.
> 
> In Xen 4.5 and earlier, the strdup does not tolerate NULL, and libxl
> crashes immediately.  This has been detected by osstest as a
> regression in Xen 4.5.
> 
> IMO this patch should be applied immediately to
>   xen.git#staging-4.5 (to check that it fixes the osstest regression)
>   xen.git#staging     (to check that it does not break master
> 
> Subject to passes, it should then be propagated to all supported
> stable trees and also be mentioned in an update to XSA-178.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: security@xenproject.org
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxl/libxl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 006b83f..7584966 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1399,7 +1399,7 @@ int libxl_evenable_disk_eject(libxl_ctx *ctx, uint32_t guest_domid,
>  
>      const char *configured_vdev;
>      rc = libxl__xs_read_checked(gc, XBT_NULL,
> -            GCSPRINTF("%s/vdev", libxl_path), &configured_vdev);
> +            GCSPRINTF("%s/dev", libxl_path), &configured_vdev);
>      if (rc) goto out;
>  
>      evg->vdev = libxl__strdup(NOGC, configured_vdev);
> -- 
> 1.7.10.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] libxl: Fix NULL pointer due to XSA-178 fix wrong XS nodename
  2016-06-08 15:11 ` Wei Liu
@ 2016-06-08 15:18   ` Ian Jackson
  0 siblings, 0 replies; 3+ messages in thread
From: Ian Jackson @ 2016-06-08 15:18 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, security, Jan Beulich

Wei Liu writes ("Re: [PATCH] libxl: Fix NULL pointer due to XSA-178 fix wrong XS nodename"):
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Thanks.  As per discussion on irc, I have now pushed this to all the
affected branches.

I think we should issue an update to XSA-178.  Given that we warned in
the advisory that the XSA-178 patches were being tested, I think we
can afford to wait until we see another set of test results.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-06-08 15:18 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-08 14:56 [PATCH] libxl: Fix NULL pointer due to XSA-178 fix wrong XS nodename Ian Jackson
2016-06-08 15:11 ` Wei Liu
2016-06-08 15:18   ` Ian Jackson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).