linux-kernel - Re: [PATCH] serial: Do not treat the IIR register as a bitfield

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <03bb0be7-3534-0f26-9f79-061282331186@schinagl.nl>
Date:   Fri, 31 Mar 2017 13:28:00 +0200
From:   Olliver Schinagl <o.schinagl@...imaker.com>
To:     Theodore Ts'o <tytso@....edu>, Vignesh R <vigneshr@...com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Laxman Dewangan <ldewangan@...dia.com>,
        Stephen Warren <swarren@...dotorg.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Alexandre Courbot <gnurou@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        "dev@...ux-sunxi.org" <dev@...ux-sunxi.org>,
        Ed Blake <ed.blake@...tec.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Alexander Sverdlin <alexander.sverdlin@...ia.com>,
        Yegor Yefremov <yegorslists@...glemail.com>,
        Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@...el.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Jason Uy <jason.uy@...adcom.com>,
        Douglas Anderson <dianders@...omium.org>,
        Peter Hurley <peter@...leysoftware.com>,
        Tony Lindgren <tony@...mide.com>,
        Thor Thayer <tthayer@...nsource.altera.com>,
        David Lechner <david@...hnology.com>,
        Jan Kiszka <jan.kiszka@...mens.com>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "sparclinux@...r.kernel.org" <sparclinux@...r.kernel.org>
Subject: Re: [PATCH] serial: Do not treat the IIR register as a bitfield

Hey Ted,

On 30-03-17 16:11, Theodore Ts'o wrote:
> While you're fixing this, there's a bug in samples/vfio-mdev/mtty.c:
>
> 		u8 ier = mdev_state->s[index].uart_reg[UART_IER];
> 		*buf = 0;
>
> 		mutex_lock(&mdev_state->rxtx_lock);
> 		/* Interrupt priority 1: Parity, overrun, framing or break */
> 		if ((ier & UART_IER_RLSI) && mdev_state->s[index].overrun)
> 			*buf |= UART_IIR_RLSI;
>
> 		/* Interrupt priority 2: Fifo trigger level reached */
> 		if ((ier & UART_IER_RDI) &&
> 		    (mdev_state->s[index].rxtx.count ==
> 		      mdev_state->s[index].intr_trigger_level))
> 			*buf |= UART_IIR_RDI;
>
> 		/* Interrupt priotiry 3: transmitter holding register empty */
> 		if ((ier & UART_IER_THRI) &&
> 		    (mdev_state->s[index].rxtx.head ==
> 				mdev_state->s[index].rxtx.tail))
> 			*buf |= UART_IIR_THRI;
>
> 		/* Interrupt priotiry 4: Modem status: CTS, DSR, RI or DCD  */
> 		if ((ier & UART_IER_MSI) &&
> 		    (mdev_state->s[index].uart_reg[UART_MCR] &
> 				 (UART_MCR_RTS | UART_MCR_DTR)))
> 			*buf |= UART_IIR_MSI;
>
> 		/* bit0: 0=> interrupt pending, 1=> no interrupt is pending */
> 		if (*buf == 0)
> 			*buf = UART_IIR_NO_INT;
>
> It's treating the UART_IIR_* fields as a bitmask which is bad enough,
> but in the "Interrupt priority 4" case, UART_IIR_MSI is zero, so
> "*buf |= UART_IIR_MSI" is a no-op.   And in the case where the modem
> status interrupt is the only thing set, *buf will be 0, and UART_IIR_NO_INT
> gets set erroneously.
>
> So this is another example of the bug of trying to treat the
> UART_IIR_* fields as a bitmask....
>
> Yes, it's only sample code, but best fix it now before it gets copied
> elsewhere and metastisizes.   :-)

Yeah, I notice that a lot of the code I modified in this patch was 
either copy pasted or 'inspired by'. So having bad examples around is 
really bad as you state!

Additionally the friendly build bot reminded me there are other 
subsystems that do this as well, so I'll update the patch to get those 
too and this one too.

Olliver

>
> 							- Ted
> 							
>