Xen devel - Dec 2012 - [PATCH] Access control in Xen privcmd_ioctl_mmap

In the privcmd Linux driver two checks in the functions
privcmd_ioctl_mmap and privcmd_ioctl_mmap_batch are not needed as they
are trying to enforce hypervisor-level access control.  They should be
removed as they break secondary control domains when performing dom0
disaggregation. Xen itself provides adequate security controls around
these hypercalls and these checks prevent those controls from
functioning as intended.

The patch applies to the stable Linux 3.7.1 kernel.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: xen-devel@lists.xensource.com
Cc: linux-kernel@vger.kernel.org
---
 drivers/xen/privcmd.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 71f5c45..adaa260 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -196,9 +196,6 @@ static long privcmd_ioctl_mmap(void __user *udata)
        LIST_HEAD(pagelist);
        struct mmap_mfn_state state;

-       if (!xen_initial_domain())
-               return -EPERM;
-
        if (copy_from_user(&mmapcmd, udata, sizeof(mmapcmd)))
                return -EFAULT;

@@ -316,9 +313,6 @@ static long privcmd_ioctl_mmap_batch(void __user
*udata, int version)
        int *err_array = NULL;
        struct mmap_batch_state state;

-       if (!xen_initial_domain())
-               return -EPERM;
-
        switch (version) {
        case 1:
                if (copy_from_user(&m, udata, sizeof(struct
privcmd_mmapbatch)))

On 12/31/2012 03:44 PM, Tamas Lengyel wrote:
> In the privcmd Linux driver two checks in the functions
> privcmd_ioctl_mmap and privcmd_ioctl_mmap_batch are not needed as they
> are trying to enforce hypervisor-level access control.  They should be
> removed as they break secondary control domains when performing dom0
> disaggregation. Xen itself provides adequate security controls around
> these hypercalls and these checks prevent those controls from
> functioning as intended.
> 
> The patch applies to the stable Linux 3.7.1 kernel.
It also applies to (and I have tested it on) 3.8-rc1.
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: xen-devel@lists.xensource.com
> Cc: linux-kernel@vger.kernel.org
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

On Mon, Dec 31, 2012 at 03:44:30PM -0500, Tamas Lengyel
wrote:
> In the privcmd Linux driver two checks in the functions
> privcmd_ioctl_mmap and privcmd_ioctl_mmap_batch are not needed as they
> are trying to enforce hypervisor-level access control.  They should be
> removed as they break secondary control domains when performing dom0
> disaggregation. Xen itself provides adequate security controls around
> these hypercalls and these checks prevent those controls from
> functioning as intended.
> 
> The patch applies to the stable Linux 3.7.1 kernel.
Hm, I get this:

atching file drivers/xen/privcmd.c
Hunk #1 FAILED at 196.
patch: **** malformed patch at line 91: *udata, int version)

Anyhow I fixed it up.

Should this patch also be back-ported to the stable trees?
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: xen-devel@lists.xensource.com
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/xen/privcmd.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 71f5c45..adaa260 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -196,9 +196,6 @@ static long privcmd_ioctl_mmap(void __user *udata)
>         LIST_HEAD(pagelist);
>         struct mmap_mfn_state state;
> 
> -       if (!xen_initial_domain())
> -               return -EPERM;
> -
>         if (copy_from_user(&mmapcmd, udata, sizeof(mmapcmd)))
>                 return -EFAULT;
> 
> @@ -316,9 +313,6 @@ static long privcmd_ioctl_mmap_batch(void __user
> *udata, int version)
>         int *err_array = NULL;
>         struct mmap_batch_state state;
> 
> -       if (!xen_initial_domain())
> -               return -EPERM;
> -
>         switch (version) {
>         case 1:
>                 if (copy_from_user(&m, udata, sizeof(struct
privcmd_mmapbatch)))

> Anyhow I fixed it up.
>
Sorry about that (first time patch submitter).

> Should this patch also be back-ported to the stable trees?

That would be great but not a requirement.


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