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
Ian Campbell
2013-Apr-24 08:24 UTC
Re: arm: Minor bug report & Fix in gic_route_irq_to_guest
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
Sengul Thomas
2013-Apr-24 08:36 UTC
Re: arm: Minor bug report & Fix in gic_route_irq_to_guest
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 > >
Julien Grall
2013-Apr-24 13:22 UTC
Re: arm: Minor bug report & Fix in gic_route_irq_to_guest
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 pointerThanks 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