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

[Sengul Thomas <thomas.sengul@xxxxxxxxx>]

On Wed, Apr 24, 2013 at 5:24 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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.

Oh, I just checked in mainline and got it!

>
> 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.

I got it, caller handles lifetime.

>
> 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