linux-kernel - Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201212061111.39057.vitas@nppfactor.kiev.ua>
Date:	Thu, 6 Dec 2012 11:11:38 +0200
From:	Vitalii Demianets <vitas@...factor.kiev.ua>
To:	"Hans J. Koch" <hjk@...sjkoch.de>
Cc:	Cong Ding <dinggnu@...il.com>, linux-kernel@...r.kernel.org,
	"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

On Thursday 06 December 2012 04:41:01 Hans J. Koch wrote:
> > @@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, 
struct uio_info *dev_info)
> >  	 * remember the state so we can allow user space to enable it later.
> >  	 */
> >  
> > -	if (!test_and_set_bit(0, &priv->flags))
> > +	if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
> >  		disable_irq_nosync(irq);
> 
> That is not safe. That flag can only be changed by userspace, and if the
> userspace part is not running, the CPU on which the irq is pending will
> hang. 
> The handler has to disable the interrupt in any case.
> (The original version had the same problem, I know...)
> 

Perhaps I'm missing something here, but I don't see your point. If userspace 
is not (implicitly) calling uio_pdrv_genirq_irqcontrol() by writing to the 
device file, then on the first invocation of interrupt handler that IRQ is 
disabled by disable_irq_nosync() and that's the end of story - no more irqs, 
no problems. Until userspace writes non-zero to the device file, of course.

> >  	priv->uioinfo = uioinfo;
> >  	spin_lock_init(&priv->lock);
> > -	priv->flags = 0; /* interrupt is enabled to begin with */
> > +	/* interrupt is enabled to begin with */
> > +	priv->flags = uioinfo_alloced ? (1 << UIO_INFO_ALLOCED) : 0;
> 
> The comment doesn't describe the line it's supposed to comment.
> 

The comment draws attention to the fact that UIO_IRQ_DISABLED is not set 
initially, and this is done on purpose. Particularly it is done because of 
the potential problem you noted earlier - if userspace is never setting this 
flag, the interrupt handler will disable irq on the first invocation thanks 
to the absence of the flag (if(!test_and_set_bit(UIO_IRQ_DISABLED,...) will 
succeed).
So, I think we really need some comment explaining how the things are arranged 
here; after all even you haven't got the whole picture on the first glance.
Maybe the current wording of the comment is not the best. I've just left what 
was there before.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/