On Thu, 2010-08-12 at 18:19 +0200, Dan Carpenter wrote:
> On Thu, Aug 12, 2010 at 05:35:04PM +0300, Maxim Levitsky wrote:
> > On Thu, 2010-08-12 at 09:46 +0200, Dan Carpenter wrote:
> > > There were a couple issues here. If the allocation failed for "dev"
> > > then it would lead to a NULL dereference. If request_irq() or
> > > request_region() failed it would release the irq and the region even
> > > though they were not successfully aquired.
> > >
> > > Signed-off-by: Dan Carpenter <error27@xxxxxxxxx>
> >
> > I don't think this is needed.
> > I just alloc all the stuff, and if one of allocations fail, I free them
> > all. {k}free on NULL pointer is perfectly legal.
> >
> > Same about IO and IRQ.
> > IRQ0 and IO 0 isn't valid, and I do test that in error path.
> >
> >
>
> Here is the original code:
>
> Here is where we set "dev".
>
> 785 dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
> 786
> 787 if (!input_dev || !ir_props || !dev)
> 788 goto error;
>
> [snip]
>
> Here is where we set the IO and IRQ:
>
> 800 dev->hw_io = pnp_port_start(pnp_dev, 0);
> 801 dev->irq = pnp_irq(pnp_dev, 0);
>
> [snip]
>
> Here is where the request_region() and request_irq() are.
>
> 806 if (!request_region(dev->hw_io, ENE_MAX_IO, ENE_DRIVER_NAME))
> 807 goto error;
> 808
> 809 if (request_irq(dev->irq, ene_isr,
> 810 IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev))
> 811 goto error;
>
> [snip]
>
> Here is the error label:
>
> 897 error:
> 898 if (dev->irq)
> ^^^^^^^^
>
> Oops! The allocation of dev failed and this is a NULL
> dereference.
>
> 899 free_irq(dev->irq, dev);
>
> Oops! Request region failed and dev->irq is non-zero but
> request_irq() hasn't been called.
>
> 900 if (dev->hw_io)
> 901 release_region(dev->hw_io, ENE_MAX_IO);
>
> Oops! dev->hw_io is non-zero but request_region() failed and so
> we just released someone else's region.
Ok, this is something different.
To be honest I was in hurry when I prepared the patch, so I didn't look
at this.
The intent was correct, and untill you pointed that out I somehow
assumed that error patch does what I supposed it to do... well...
In few days I will switch back to this driver and fix this problem.
I also have quite a lot of work to do in this driver now that I have
some hardware documentation (register renames are the fun part...).
So thanks for catching this.
Best regards,
Maxim Levitsky
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
