Re: [Xen-devel] arm: Minor bug report & Fix in gic_route_irq_to_guest

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

Adding Julien, in general it's a good idea to CC the appropriate
maintainers where possible.

On Wed, 2013-04-24 at 03:24 +0100, Sengul Thomas wrote:
> Hello,
> 
> I found that when calling gic_route_irq_to_guest in construct_dom0 function,
> it uses local variable "name" for passing devname argument.
> And, gic_route_irq_to_guest just copies the pointer of this devname
> and afterward,
> reading this devname gives data abort.
> 
> Here goes a simple fix: just copying the data, not the pointer
> 
> ps. I'm writing this patch on top of the following source
>     repo: git://xenbits.xen.org/people/julieng/xen-unstable.git
>     branch: arndale
> and, I'm curious is it ok?

I guess this is specific to Julien's branch because in mainline all of
the devname arguments are string literals.

I think rather than casting away the const it should be up to the caller
of gic_route_irq_to_guest to ensure that the devname it passes in has
the appropriate lifetime -- i.e. it needs to do the copy itself.
Likewise the caller of release_irq would need to free it, but in this
case I don't think we would ever release this IRQ.

Ian.

> 
> Signed-off-by: Thomas Sengul <thomas.sengul@xxxxxxxxx>
> ---
>  xen/arch/arm/gic.c |   13 ++++++++++++-
>  xen/arch/arm/irq.c |   14 +++++++++++++-
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 63caeb8..012aae9 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -468,7 +468,10 @@ void __init release_irq(unsigned int irq)
>      do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
> 
>      if (action && action->free_on_release)
> +    {
> +        xfree((void *)action->name);
>          xfree(action);
> +    }
>  }
> 
>  static int __setup_irq(struct irq_desc *desc, unsigned int irq,
> @@ -617,13 +620,20 @@ int gic_route_irq_to_guest(struct domain *d,
> unsigned int irq,
>      struct irq_desc *desc = irq_to_desc(irq);
>      unsigned long flags;
>      int retval;
> +    char *name;
> 
>      action = xmalloc(struct irqaction);
>      if (!action)
>          return -ENOMEM;
> 
>      action->dev_id = d;
> -    action->name = devname;
> +
> +#define MIN_ACTION_NAME_LEN 16
> +    name = xmalloc_array(char, MIN_ACTION_NAME_LEN);
> +    if (!name)
> +        return -ENOMEM;
> +    strlcpy(name, devname, strnlen(devname, MIN_ACTION_NAME_LEN));
> +    action->name = name;
> 
>      spin_lock_irqsave(&desc->lock, flags);
>      spin_lock(&gic.lock);
> @@ -635,6 +645,7 @@ int gic_route_irq_to_guest(struct domain *d,
> unsigned int irq,
> 
>      retval = __setup_irq(desc, irq, action);
>      if (retval) {
> +        xfree((void *)action->name);
>          xfree(action);
>          goto out;
>      }
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 8c96a0a..e6c24f9 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -99,6 +99,7 @@ int __init request_irq(unsigned int irq,
>  {
>      struct irqaction *action;
>      int retval;
> +    char *name;
> 
>      /*
>       * Sanity-check: shared interrupts must pass in a real dev-ID,
> @@ -116,13 +117,24 @@ int __init request_irq(unsigned int irq,
>          return -ENOMEM;
> 
>      action->handler = handler;
> -    action->name = devname;
> +
> +#define MIN_ACTION_NAME_LEN 16
> +    name = xmalloc_array(char, MIN_ACTION_NAME_LEN);
> +    if (!name)
> +        return -ENOMEM;
> +    strlcpy(name, devname, strnlen(devname, MIN_ACTION_NAME_LEN));
> +    action->name = name;
> +
>      action->dev_id = dev_id;
>      action->free_on_release = 1;
> 
>      retval = setup_irq(irq, action);
>      if (retval)
> +    {
> +        xfree((void *)action->name);
>          xfree(action);
> +    }
> +
> 
>      return retval;
>  }
> 
> 
> Sincerely,
> Thomas
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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