linux-kernel - [BUG]: inverse lock depedency in video-buf.c

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 11 Sep 2007 15:01:19 +0300
From:	Maxim Levitsky <maximlevitsky@...il.com>
To:	mchehab@...radead.org
Cc:	v4l-dvb-maintainer@...uxtv.org, video4linux-list@...hat.com,
	linux-kernel@...r.kernel.org
Subject: [BUG]: inverse lock depedency in video-buf.c

Hi,

I found a serious bug in video-buf.c.
It was already reported long time ago but got unnoticed.
see: http://www.ussg.iu.edu/hypermail/linux/kernel/0608.2/1637.html

I reported this two days ago, but I am writing again, since I understand it a lot better.

So it is a lock inversion, between calling munmap()/mmap() on a video buffer, and caling VIDIOC_QBUF to activate it.

munmap takes current->mm->mmap_sem (and it has to), but then videobuf_vm_close has to take q->lock, since it modify buffer queue.
on the other hand videobuf_qbuf takes q->lock (and it has to take it) and then calls q->ops->buf_prepare which supposed to do 
some card-specific initialization, and it locks it in memory with videobuf_iolock, but that requires allocating user pages, 
so current->mm->mmap_sem is taken.

I don't yet see a simple solution.
What I thought about includes this:

1) take a current->mm->mmap_sem _before_ q->lock in all functions that might call q->ops->buf_prepare:
it works, but is ugly, and unreliable since q->ops->buf_prepare can be called directly by driver code (saa7134 does that)

2) move locking of current->mm->mmap_sem from videobuf_dma_init_user to videobuf_iolock, and just try to take it, 
if it is not aviable, unlock q->lock, wait for munmap/mmap to finish, and then recheck what munmap did:
works, but again not a good solution, since current->mm->mmap_sem can be taken by something else, and thus releasing q->lock will
allow a random videobuf function to complete, and this is incorrect.

3) Lock memory at mmap time:
Seems fine, but will allocate more memory (if app mmaps buffers, but don't use them), and at mmap time I dont know r/w direction,
but it seems to be unused
but that will help only with munmap case, but mmap will still race with VIDIOC_QBUF.

4) Your idea/patch goes here :-)

Best regards,
	Maxim Levitsky
-
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/