linux-kernel - [RFC] ->poll() sparse annotations

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>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 18 Jul 2017 02:36:05 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Christoph Hellwig <hch@....de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: [RFC] ->poll() sparse annotations

[davem Cc'd, due to sparc/sockets problem involved]

On Wed, Jul 05, 2017 at 11:38:21PM +0100, Al Viro wrote:

> A side note right back at you - POLL... stuff.  I'd redone the old
> "hunt the buggy ->poll() instances down" series (took about 12 hours
> total), got it to the point where all remaining sparse warnings about
> that type are for genuine bugs.  It goes like that:
> 
> 	define __poll_t, annotate constants
> Type is controlled by ifdef - it's unsigned int unless CHECK_POLL is
> defined and a bitwise type otherwise.
> 	->poll() methods should return __poll_t
> 	anntotate the places where ->poll() return values go
> 	annotate poll-related wait keys
> 	annotate poll_table_struct ->_key
> That ends all infrastructure work.  Methods declarations are annotated,
> instances are *not*.  Due to that ifdef CHECK_POLL, normal builds, including
> normal sparse builds, are unaffected; with CF=-DCHECK_POLL you get __poll_t
> warnings.

FWIW, I've just updated that queue to -rc1.  The branch is currently at #misc.poll
and it's fairly close to "all warnings left are genuine".

drivers/media/pci/saa7164/saa7164-vbi.c:632:24:    expected restricted __poll_t
drivers/media/pci/saa7164/saa7164-vbi.c:637:40:    expected restricted __poll_t
drivers/media/pci/saa7164/saa7164-vbi.c:647:40:    expected restricted __poll_t
drivers/media/platform/exynos-gsc/gsc-m2m.c:718:24:    expected restricted __poll_t
drivers/media/platform/s3c-camif/camif-capture.c:602:21:    expected restricted __poll_t [usertype] ret
drivers/media/radio/radio-wl1273.c:1088:24:    expected restricted __poll_t
drivers/platform/goldfish/goldfish_pipe.c:549:24:    expected restricted __poll_t
drivers/tty/n_r3964.c:1241:24:    expected restricted __poll_t [assigned] [usertype] result
drivers/uio/uio.c:505:24:    expected restricted __poll_t
kernel/trace/ring_buffer.c:640:32:    expected restricted __poll_t
sound/core/seq/oss/seq_oss.c:206:24:    expected restricted __poll_t
sound/core/seq/seq_clientmgr.c:1092:24:    expected restricted __poll_t

These are ->poll() instances returning -E... in some cases.  All genuine bugs.

kernel/events/core.c:4561:24:    expected restricted __poll_t [usertype] events
kernel/events/ring_buffer.c:22:39:    got restricted __poll_t [usertype] <noident>

atomic_{set,xchg}() used on __poll_t; false positives, these two.

drivers/media/i2c/saa6588.c:416:35:    right side has type restricted __poll_t
drivers/media/pci/bt8xx/bttv-driver.c:3347:20:    got restricted __poll_t [assigned] [usertype] res
drivers/media/pci/bt8xx/bttv-driver.c:3350:19:    expected restricted __poll_t
drivers/media/pci/saa7134/saa7134-video.c:1243:16: warning: restricted __poll_t degrades to integer
drivers/media/pci/saa7134/saa7134-video.c:1243:19:    expected restricted __poll_t

saa6588_ioctl(, SAA6588_CMD_POLL, ) stores POLL... bitmap in the same field
where other subfunctions store int.  Could be annotated away (union in the
structure being filled), but... not much point, TBH.  Ugly misannotation,
but no more than that.

fs/fuse/file.c:2761:25: warning: cast from restricted __poll_t
fs/fuse/file.c:2783:30:    expected restricted __poll_t

fuse puts POLL... bitmaps on the wire.  That's a problem waiting to happen,
in theory - different architectures have different encodings.

fs/eventpoll.c:1168:28: warning: restricted __poll_t degrades to integer
fs/eventpoll.c:1208:57: warning: restricted __poll_t degrades to integer
fs/eventpoll.c:1212:57: warning: restricted __poll_t degrades to integer
fs/eventpoll.c:2054:49: warning: restricted __poll_t degrades to integer
fs/eventpoll.c:2114:37:    right side has type restricted __poll_t
fs/eventpoll.c:2130:45:    right side has type restricted __poll_t
fs/eventpoll.c:880:18:    expected restricted __poll_t [usertype] _key
fs/eventpoll.c:880:18:    expected restricted __poll_t [usertype] _key
fs/eventpoll.c:880:18:    expected restricted __poll_t [usertype] _key
fs/eventpoll.c:880:18:    expected restricted __poll_t [usertype] _key
fs/eventpoll.c:882:41: warning: restricted __poll_t degrades to integer
fs/eventpoll.c:882:41: warning: restricted __poll_t degrades to integer
fs/eventpoll.c:882:41: warning: restricted __poll_t degrades to integer
fs/eventpoll.c:882:41: warning: restricted __poll_t degrades to integer
fs/eventpoll.c:895:39:    got restricted __poll_t
fs/eventpoll.c:951:32:    expected restricted __poll_t

A bloody mess.  This is a genuine ABI problem, and I'm not sure if it
_can_ be fixed.  struct epoll_event contains a field with or'ed
EPOLL... constants.  They are defined in libc and are arch-independent.
The kernel assumes that POLL... constants are equal to corresponding
EPOLL... ones.  Unfortunately, that is not true.  First POLL... are
universal and do match EPOLL...; however, starting with POLLWRNORM they
diverge on quite a few architectures.

	     common  bfin,frv,m68k,mips       xtensa  sparc
WRNORM bit	8	2			2	2
WRBAND bit	9	8			8	8
MSG bit		10	10			10	9
REMOVE bit	12	12			11	10
RDHUP bit	13	13			13	11

Now, POLLREMOVE doesn't have EPOLL... equivalent, but others
do.  As the result, blackfin, frv, m68k, mips and xtensa have
EPOLLWRNORM matching POLLWRBAND and EPOLLWRBAND not matching
anything.  sparc has EPOLLWRNORM matching POLLWRBAND, EPOLLWRBAND
matching POLLMSG (and never triggered), EPOLLMSG matching POLLREMOVE
(and also never triggered) and EPOLLRDHUP not matching anything.

I don't believe that anything tries to use EPOLLMSG; EPOLLWRBAND
and EPOLLWRNORM might be used (even though our manpage doesn't
document either).  EPOLLRDHUP _is_ documented and flat-out does
not work on sparc; the only way to catch POLLRDHUP via epoll
there is to give it a value that is not any of EPOLL... constants.
Hell knows if anything tries to do it there...

Comments?