arm: Minor bug report & Fix in gic_route_irq_to_guest

arm: Minor bug report & Fix in gic_route_irq_to_guest

[Sengul Thomas]

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?


Signed-off-by: Thomas Sengul <thomas.sengul@gmail.com>
---
 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

Re: arm: Minor bug report & Fix in gic_route_irq_to_guest

[Ian Campbell]

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@gmail.com>
> ---
>  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@lists.xen.org
> http://lists.xen.org/xen-devel

Re: arm: Minor bug report & Fix in gic_route_irq_to_guest

[Sengul Thomas]

On Wed, Apr 24, 2013 at 5:24 PM, Ian Campbell <Ian.Campbell@citrix.com> 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@gmail.com>
>> ---
>>  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@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>

Re: arm: Minor bug report & Fix in gic_route_irq_to_guest

[Julien Grall]

On 04/24/2013 03:24 AM, 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

Thanks for this report. I prefer to remove all uses of local variable
"name", because I intend to remove this code soon.

I have pushed the commit in the arndale branch with another minor change.

Cheers,

Julien

> Signed-off-by: Thomas Sengul <thomas.sengul@gmail.com>
> ---
>  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@lists.xen.org
> http://lists.xen.org/xen-devel