From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Marczykowski Subject: Re: [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics Date: Wed, 08 May 2013 01:56:15 +0200 Message-ID: <5189949F.3030803@invisiblethingslab.com> References: <1367225324.3142.216.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8806039362459500168==" Return-path: In-Reply-To: <1367225324.3142.216.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --===============8806039362459500168== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="----enig2KRJRGUJXLXPJTEUQOMMR" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2KRJRGUJXLXPJTEUQOMMR Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 29.04.2013 10:48, Ian Campbell wrote: > On Sun, 2013-04-28 at 00:12 +0100, Marek Marczykowski wrote: >> One more place where code assumed that all backends are in dom0. List >> devices in domain device/ tree, instead of backend/ of dom0. >> Additionally fix libxl_devid_to_device_{nic,disk} to fill backend_domi= d >> properly. >> >> Signed-off-by: Marek Marczykowski >> --- >> tools/libxl/libxl.c | 71 ++++++++++++++++++++++++++++++++++++++------= --------- >> 1 file changed, 51 insertions(+), 20 deletions(-) >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index c386004..e2c678a 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -2308,7 +2308,7 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, ui= nt32_t domid, >> const char *vdev, libxl_device_disk *di= sk) >> { >> GC_INIT(ctx); >> - char *dompath, *path; >> + char *dompath, *path, *backend_domid; >> int devid =3D libxl__device_disk_dev_number(vdev, NULL, NULL); >> int rc =3D ERROR_FAIL; >> =20 >> @@ -2328,6 +2328,13 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, u= int32_t domid, >> goto out; >> =20 >> rc =3D libxl__device_disk_from_xs_be(gc, path, disk); >> + >> + backend_domid =3D libxl__xs_read(gc, XBT_NULL, >> + libxl__sprintf(gc, "%s/device/vbd/%d/backend-id", >> + dompath, devid)); >> + if (backend_domid) >> + disk->backend_domid =3D atoi(backend_domid); >=20 > I think this should be folded into ..._from_xs_be, either by parsing th= e > path argument or by doing the appropriate lookup via the frontend-path > node inside the function. Since I guess this will be common to both NIC= > and disk perhaps a helper for from_xs_be to call would be appropriate? And perhaps with getting backend path from frontend device, which will sa= ve one additional duplicated line of code. More on this below. > In general we prefer to avoid relying on frontend owned state (makes it= > easier to reason about the safety if we just don't do it, although > obviously we sometimes have to, and this may be one of those cases). I'm afraid this is the case. Domains (both backend and frontend) can cont= rol content of device directory, but not existence of directory itself (with obvious exception of dom0 aka toolstack domain). So if we check if device= paths - both frontend and backend - points to each other, it should be reasonably safe. (...) > If you push this down into from_xs_be then you avoid duplicating this > logic. >=20 > NB have you checked that from_xs_be is robust against a potentially > malicious backend path, since the frontend now controls where it goes > and could redirect it to a directory which it controls? > > Simple things like allowing for missing keys which "must" be there. I > wonder if an owner + permissions check on the backend directory would b= e > a good idea, i.e. parse the path to get the backend id and insist that > it owns the directory? And checking if $be_path/frontend points back to the right device. This means *_from_xs_be needs original frontend path (or at least fronten= d domid), which means it should be really *_from_xs_fe. The general workflo= w would be: 1. get backend device path 2. get backend domid 3. check if backend domid matches backend device path (enforcing /local/domain/%d/backend scheme) 4. check if backend domid owns backend device path (redundant with previo= us one?) 5. check if backend/frontend entry points back to original frontend 6. proceed to original *_from_xs_be Items 1-5 can be folded into common helper, called at the beginning of ev= ery *_from_xs_fe function. What do you think? --=20 Best Regards, Marek Marczykowski Invisible Things Lab ------enig2KRJRGUJXLXPJTEUQOMMR Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJRiZSfAAoJENuP0xzK19csRfcH/iylgw+LXPEC118c2GFNHeGD wHwjoRxC1N/ybdz2jBoz+YnhD4pmqY+AMs65DJWoDfLke77kkn1s7AzvMw41c1NB ElDQXaONh6r/IE4q1ghjHeSVB/beqZ1Uh94+T0tbEjBA1XXK22vxKsp7dfHrMTFS oYp0hXEuaMQb85WbIn+WSBQUn+B1xRbkY4spjq/oxOaNJdhZFv9aSoH1+DoPPD0J OAXj4ncvl8sUmMsVlahlx0kbfR+yV2chqIK0+V3VJufUIIsc7UKE1A6PsK5PRSP3 aopf3elKfNh4tYcyYPea8o0igiY7MJeCK8WDaQguuJwmFPfBwgQE/UwfhTI4FPw= =zDKh -----END PGP SIGNATURE----- ------enig2KRJRGUJXLXPJTEUQOMMR-- --===============8806039362459500168== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============8806039362459500168==--