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
| ||
|
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/
Powered by blists - more mailing lists