|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xsm_remove_from_physmap is (so far) only defined for X86 architecture, not for ARM
On 01/15/2013 06:18 AM, Ian Campbell wrote:
> On Fri, 2013-01-11 at 17:37 +0000, Daniel De Graaf wrote:
>> On 01/11/2013 11:46 AM, Ian Campbell wrote:
>>> On Fri, 2013-01-11 at 16:36 +0000, Keir Fraser wrote:
>>>> On 11/01/2013 16:24, "Lars Rasmusson" <lra@xxxxxxx> wrote:
>>>>
>>>>>> On 11/01/2013 13:32, "lra@xxxxxxx" <lra@xxxxxxx> wrote:
>>>>>>
>>>>>>> From: Lars Rasmusson <Lars.Rasmusson@xxxxxxx>
>>>>>>>
>>>>>>> Signed-off-by: Lars Rasmusson <Lars.Rasmusson@xxxxxxx>
>>>>>>
>>>>>> If this is a build fix after my checkins this morning then:
>>>>>> Acked-by: Keir Fraser <keir@xxxxxxx>
>>>>>
>>>>> Yes, the XEN_TARGET_ARCH=arm32 make breaks when compiling memory.c
>>>>>
>>>>> In xen/include/xsm/dummy.h where many of the functions are used, some are
>>>>> declared only for X86, so I picked the same #ifdef CONFIG_X86
>>>>> as the header file uses.
>>>>>
>>>>> As Ian said, it's not pretty, but since ARM doesn't have xsm (yet?) I
>>>>> think
>>>>> adding a dummy xsm_remove_from_physmap for arm also is ugly.
>>>>>
>>>>> Is there some other way to write memory.c so that it doesn't need
>>>>> xsm_remove...? (I mean, it does't need xsm_add....)
>>>>
>>>> The XSM infrastructure is not architecture dependent. It's probably a
>>>> mistake that xsm_remove_from_physmap() is ifdef CONFIG_X86.
>>>
>>> Agreed.
>>>
>>> 8<---------------------
>>>
>>> >From fb57be285e956cadea51dc48a28fba77d752044d Mon Sep 17 00:00:00 2001
>>> From: Ian Campbell <ian.campbell@xxxxxxxxxx>
>>> Date: Fri, 11 Jan 2013 16:44:14 +0000
>>> Subject: [PATCH] xen arm: add XSM hooks to arch_memory_op
>>>
>>> Treat XENMEM_add_to_physmap_range the same as
>>> XENMEM_add_to_physmap.
>>>
>>> Reported-by: Lars Rasmusson <Lars.Rasmusson@xxxxxxx>
>>> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
>>> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>>> ---
>>> xen/arch/arm/mm.c | 13 +++++++++++++
>>> xen/include/xsm/dummy.h | 24 ++++++++++++------------
>>> 2 files changed, 25 insertions(+), 12 deletions(-)
>>
>> I agree with the movement (neither of these hooks are x86-specific), but
>> this patch also needs to move the hooks in xen/include/xsm/xsm.h and
>> xen/xsm/flask/hooks.c out of the same #ifdefs.
>
> Thanks, and dummy.c.
>
> Turns out this is far from the only barrier to enabling FLASK on ARM,
> flask_op.c has a few x86isms not included in #ifdef, e.g. MSIs
> I also needed a few extra #includes on ARM for things which are picked
> up indirectly on x86.
>
> The patch is below, its a bit ugly and totally untested. I could pick
> out the bits to do the rigth thing on ARM without FLASK on if that would
> be preferred.
>
>> On a mostly unrelated note, Ian: do you think it would be useful to add
>> build tests with XSM enabled to catch these types of issues during the
>> automated testing? It just requires "echo XSM_ENABLE=y > .config"
>> before the build, and is apparently best done for both x86 and arm.
>
> In general I think build tests with a variety of different compile time
> options, xsm being one of them, would be very nice to have. e.g. I'd
> also like to get at least a build test of ARM into the system prior to
> suitable hardware arriving later in the year.
>
> Perhaps a flight of build tests could be added?
>
> Ian.
>
> 8<----------------
>
>>From 469d20054b96b8bc749891c366659532b67d6031 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Date: Fri, 11 Jan 2013 16:44:14 +0000
> Subject: [PATCH] xen arm: add XSM hooks to arch_memory_op
>
> Treat XENMEM_add_to_physmap_range the same as XENMEM_add_to_physmap.
>
> Also conditionalise more x86-isms and add required headers such that
> it compiles on ARM. Totally untested.
>
> Reported-by: Lars Rasmusson <Lars.Rasmusson@xxxxxxx>
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> ---
> xen/arch/arm/mm.c | 13 +++++++++++++
> xen/include/xsm/dummy.h | 25 +++++++++++++------------
> xen/include/xsm/xsm.h | 12 ++++++------
> xen/xsm/dummy.c | 5 +++--
> xen/xsm/flask/avc.c | 1 +
> xen/xsm/flask/flask_op.c | 4 +++-
> xen/xsm/flask/hooks.c | 44 ++++++++++++++++++++++++++++++--------------
> xen/xsm/xsm_policy.c | 1 +
> 8 files changed, 70 insertions(+), 35 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index d97b3ea..311ec97 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -35,6 +35,7 @@
> #include <asm/current.h>
> #include <public/memory.h>
> #include <xen/sched.h>
> +#include <xsm/xsm.h>
>
> struct domain *dom_xen, *dom_io, *dom_cow;
>
> @@ -651,6 +652,12 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void)
> arg)
> if ( rc != 0 )
> return rc;
>
> + if ( xsm_add_to_physmap(XSM_TARGET, current->domain, d) )
> + {
> + rcu_unlock_domain(d);
> + return -EPERM;
> + }
> +
Using the return value of the xsm hook instead of -EPERM is preferred; this
turns
an -ENOMEM inside FLASK into an -EPERM (and same below).
> rc = xenmem_add_to_physmap_one(d, xatp.space, DOMID_INVALID,
> xatp.idx, xatp.gpfn);
>
> @@ -671,6 +678,12 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void)
> arg)
> if ( rc != 0 )
> return rc;
>
> + if ( xsm_add_to_physmap(XSM_TARGET, current->domain, d) )
> + {
> + rcu_unlock_domain(d);
> + return -EPERM;
> + }
> +
> rc = xenmem_add_to_physmap_range(d, &xatpr);
>
> rcu_unlock_domain(d);
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 1ca82b0..f40b196 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -16,6 +16,7 @@
> */
>
> #include <xen/sched.h>
> +#include <xen/errno.h>
> #include <xsm/xsm.h>
>
> /* Cannot use BUILD_BUG_ON here because the expressions we check are not
> @@ -443,6 +444,18 @@ static XSM_INLINE int
> xsm_pci_config_permission(XSM_DEFAULT_ARG struct domain *d
> return xsm_default_action(action, current->domain, d);
> }
>
> +static XSM_INLINE int xsm_add_to_physmap(XSM_DEFAULT_ARG struct domain *d1,
> struct domain *d2)
> +{
> + XSM_ASSERT_ACTION(XSM_TARGET);
> + return xsm_default_action(action, d1, d2);
> +}
> +
> +static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain
> *d1, struct domain *d2)
> +{
> + XSM_ASSERT_ACTION(XSM_TARGET);
> + return xsm_default_action(action, d1, d2);
> +}
> +
> #ifdef CONFIG_X86
> static XSM_INLINE int xsm_shadow_control(XSM_DEFAULT_ARG struct domain *d,
> uint32_t op)
> {
> @@ -544,18 +557,6 @@ static XSM_INLINE int
> xsm_update_va_mapping(XSM_DEFAULT_ARG struct domain *d, st
> return xsm_default_action(action, d, f);
> }
>
> -static XSM_INLINE int xsm_add_to_physmap(XSM_DEFAULT_ARG struct domain *d1,
> struct domain *d2)
> -{
> - XSM_ASSERT_ACTION(XSM_TARGET);
> - return xsm_default_action(action, d1, d2);
> -}
> -
> -static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain
> *d1, struct domain *d2)
> -{
> - XSM_ASSERT_ACTION(XSM_TARGET);
> - return xsm_default_action(action, d1, d2);
> -}
> -
> static XSM_INLINE int xsm_bind_pt_irq(XSM_DEFAULT_ARG struct domain *d,
> struct xen_domctl_bind_pt_irq *bind)
> {
> XSM_ASSERT_ACTION(XSM_HOOK);
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 8947372..5048344 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -90,6 +90,7 @@ struct xsm_operations {
> int (*memory_adjust_reservation) (struct domain *d1, struct domain *d2);
> int (*memory_stat_reservation) (struct domain *d1, struct domain *d2);
> int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct
> page_info *page);
> + int (*add_to_physmap) (struct domain *d1, struct domain *d2);
> int (*remove_from_physmap) (struct domain *d1, struct domain *d2);
>
> int (*console_io) (struct domain *d, int cmd);
> @@ -149,7 +150,6 @@ struct xsm_operations {
> struct domain *f, uint32_t flags);
> int (*mmuext_op) (struct domain *d, struct domain *f);
> int (*update_va_mapping) (struct domain *d, struct domain *f,
> l1_pgentry_t pte);
> - int (*add_to_physmap) (struct domain *d1, struct domain *d2);
> int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq
> *bind);
> int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq
> *bind);
> int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e,
> uint8_t allow);
> @@ -335,6 +335,11 @@ static inline int xsm_memory_pin_page(xsm_default_t def,
> struct domain *d1, stru
> return xsm_ops->memory_pin_page(d1, d2, page);
> }
>
> +static inline int xsm_add_to_physmap(xsm_default_t def, struct domain *d1,
> struct domain *d2)
> +{
> + return xsm_ops->add_to_physmap(d1, d2);
> +}
> +
> static inline int xsm_remove_from_physmap(xsm_default_t def, struct domain
> *d1, struct domain *d2)
> {
> return xsm_ops->remove_from_physmap(d1, d2);
> @@ -558,11 +563,6 @@ static inline int xsm_update_va_mapping(xsm_default_t
> def, struct domain *d, str
> return xsm_ops->update_va_mapping(d, f, pte);
> }
>
> -static inline int xsm_add_to_physmap(xsm_default_t def, struct domain *d1,
> struct domain *d2)
> -{
> - return xsm_ops->add_to_physmap(d1, d2);
> -}
> -
> static inline int xsm_bind_pt_irq(xsm_default_t def, struct domain *d,
> struct
> xen_domctl_bind_pt_irq *bind)
> {
> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
> index 529a724..5031e16 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -101,6 +101,9 @@ void xsm_fixup_ops (struct xsm_operations *ops)
>
> set_to_dummy_if_null(ops, do_xsm_op);
>
> + set_to_dummy_if_null(ops, add_to_physmap);
> + set_to_dummy_if_null(ops, remove_from_physmap);
> +
> #ifdef CONFIG_X86
> set_to_dummy_if_null(ops, shadow_control);
> set_to_dummy_if_null(ops, hvm_param);
> @@ -118,8 +121,6 @@ void xsm_fixup_ops (struct xsm_operations *ops)
> set_to_dummy_if_null(ops, mmu_update);
> set_to_dummy_if_null(ops, mmuext_op);
> set_to_dummy_if_null(ops, update_va_mapping);
> - set_to_dummy_if_null(ops, add_to_physmap);
> - set_to_dummy_if_null(ops, remove_from_physmap);
> set_to_dummy_if_null(ops, bind_pt_irq);
> set_to_dummy_if_null(ops, unbind_pt_irq);
> set_to_dummy_if_null(ops, ioport_permission);
> diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c
> index 7fede00..f994fd9 100644
> --- a/xen/xsm/flask/avc.c
> +++ b/xen/xsm/flask/avc.c
> @@ -28,6 +28,7 @@
> #include <xen/rcupdate.h>
> #include <asm/atomic.h>
> #include <asm/current.h>
> +#include <public/event_channel.h>
> #include <public/xsm/flask_op.h>
>
> #include "avc.h"
> diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c
> index 4426ab9..f9b16f2 100644
> --- a/xen/xsm/flask/flask_op.c
> +++ b/xen/xsm/flask/flask_op.c
> @@ -10,9 +10,11 @@
>
> #include <xen/errno.h>
> #include <xen/event.h>
> +#include <xen/init.h>
> #include <xsm/xsm.h>
> #include <xen/guest_access.h>
>
> +#include <public/event_channel.h>
> #include <public/xsm/flask_op.h>
>
> #include <avc.h>
> @@ -71,7 +73,7 @@ static int domain_has_security(struct domain *d, u32 perms)
> perms, NULL);
> }
>
> -static int flask_copyin_string(XEN_GUEST_HANDLE_PARAM(char) u_buf, char
> **buf, uint32_t size)
> +static int flask_copyin_string(XEN_GUEST_HANDLE(char) u_buf, char **buf,
> uint32_t size)
> {
> char *tmp = xmalloc_bytes(size + 1);
> if ( !tmp )
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index ba67502..c39f129 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -19,11 +19,15 @@
> #include <xen/errno.h>
> #include <xen/guest_access.h>
> #include <xen/xenoprof.h>
> +#ifdef CONFIG_X86
> #include <asm/msi.h>
> +#endif
> +#include <asm/irq.h>
> #include <public/xen.h>
> #include <public/physdev.h>
> #include <public/platform.h>
>
> +#include <public/event_channel.h>
> #include <public/xsm/flask_op.h>
>
> #include <avc.h>
> @@ -100,7 +104,9 @@ static int domain_has_xen(struct domain *d, u32 perms)
>
> static int get_irq_sid(int irq, u32 *sid, struct avc_audit_data *ad)
> {
> +#ifdef CONFIG_X86
> struct irq_desc *desc = irq_to_desc(irq);
> +#endif
> if ( irq >= nr_irqs || irq < 0 )
> return -EINVAL;
> if ( irq < nr_static_irqs ) {
> @@ -110,6 +116,7 @@ static int get_irq_sid(int irq, u32 *sid, struct
> avc_audit_data *ad)
> }
> return security_irq_sid(irq, sid);
> }
> +#ifdef CONFIG_X86
> if ( desc->msi_desc ) {
> struct pci_dev *dev = desc->msi_desc->dev;
> u32 sbdf = (dev->seg << 16) | (dev->bus << 8) | dev->devfn;
> @@ -119,6 +126,7 @@ static int get_irq_sid(int irq, u32 *sid, struct
> avc_audit_data *ad)
> }
> return security_device_sid(sbdf, sid);
> }
> +#endif
> if (ad) {
> AVC_AUDIT_DATA_INIT(ad, IRQ);
> ad->irq = irq;
> @@ -822,7 +830,9 @@ static int flask_map_domain_pirq (struct domain *d, int
> irq, void *data)
> {
> u32 sid, dsid;
> int rc = -EPERM;
> +#ifdef CONFIG_X86
> struct msi_info *msi = data;
> +#endif
> struct avc_audit_data ad;
>
> rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD);
> @@ -830,12 +840,17 @@ static int flask_map_domain_pirq (struct domain *d, int
> irq, void *data)
> if ( rc )
> return rc;
>
> - if ( irq >= nr_static_irqs && msi ) {
> +#ifdef CONFIG_X86
> + if ( irq >= nr_static_irqs && msi )
> + {
> u32 machine_bdf = (msi->seg << 16) | (msi->bus << 8) | msi->devfn;
> AVC_AUDIT_DATA_INIT(&ad, DEV);
> ad.device = machine_bdf;
> rc = security_device_sid(machine_bdf, &sid);
> - } else {
> + }
> + else
> +#endif
> + {
> rc = get_irq_sid(irq, &sid, &ad);
> }
> if ( rc )
> @@ -1055,6 +1070,16 @@ static inline int flask_tmem_control(void)
> return domain_has_xen(current->domain, XEN__TMEM_CONTROL);
> }
>
> +static int flask_add_to_physmap(struct domain *d1, struct domain *d2)
> +{
> + return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
> +}
> +
> +static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
> +{
> + return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
> +}
> +
> #ifdef CONFIG_X86
> static int flask_shadow_control(struct domain *d, uint32_t op)
> {
> @@ -1325,16 +1350,6 @@ static int flask_update_va_mapping(struct domain *d,
> struct domain *f,
> return domain_has_perm(d, f, SECCLASS_MMU, map_perms);
> }
>
> -static int flask_add_to_physmap(struct domain *d1, struct domain *d2)
> -{
> - return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
> -}
> -
> -static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
> -{
> - return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
> -}
> -
> static int flask_get_device_group(uint32_t machine_bdf)
> {
> u32 rsid;
> @@ -1501,6 +1516,9 @@ static struct xsm_operations flask_ops = {
>
> .do_xsm_op = do_flask_op,
>
> + .add_to_physmap = flask_add_to_physmap,
> + .remove_from_physmap = flask_remove_from_physmap,
> +
> #ifdef CONFIG_X86
> .shadow_control = flask_shadow_control,
> .hvm_param = flask_hvm_param,
> @@ -1518,8 +1536,6 @@ static struct xsm_operations flask_ops = {
> .mmu_update = flask_mmu_update,
> .mmuext_op = flask_mmuext_op,
> .update_va_mapping = flask_update_va_mapping,
> - .add_to_physmap = flask_add_to_physmap,
> - .remove_from_physmap = flask_remove_from_physmap,
> .get_device_group = flask_get_device_group,
> .test_assign_device = flask_test_assign_device,
> .assign_device = flask_assign_device,
> diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c
> index a419cf4..c1a3fc9 100644
> --- a/xen/xsm/xsm_policy.c
> +++ b/xen/xsm/xsm_policy.c
> @@ -20,6 +20,7 @@
>
> #include <xsm/xsm.h>
> #include <xen/multiboot.h>
> +#include <xen/init.h>
> #include <asm/bitops.h>
>
> char *__initdata policy_buffer = NULL;
>
The rest of the changes look correct. The #ifdefs are a bit ugly, but
refactoring the MSI code into an arch-specific function should fix that.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |