linux-kernel - Re: [PATCH] Input: zforce_ts

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: <20150727214442.GB3613@dtor-ws>
Date:	Mon, 27 Jul 2015 14:44:42 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Heiko Stübner <heiko.stuebner@...com>
Cc:	linux-input@...r.kernel.org, Dirk Behme <dirk.behme@...bosch.com>,
	Oleksij Rempel <external.Oleksij.Rempel@...bosch.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Input: zforce_ts - fix playload length check

On Mon, Jul 27, 2015 at 11:35:23PM +0200, Heiko Stübner wrote:
> Hi Dmitry,
> 
> Am Montag, 27. Juli 2015, 14:06:19 schrieb Dmitry Torokhov:
> > Commit 7d01cd261c76f95913c81554a751968a1d282d3a ("Input: zforce - don't
> > overwrite the stack") attempted to add a check for payload size being too
> > large for the supplied buffer. Unfortunately with the currently selected
> > buffer size the comparison is always false as buffer size is larger than
> > the value a single byte can hold, and that results in compiler warnings.
> > Additionally the check was incorrect as it was not accounting for the
> > already read 2 bytes of data stored in the buffer.
> > 
> > Fixes: 7d01cd261c76f95913c81554a751968a1d282d3a
> > Reported-by: kbuild test robot <fengguang.wu@...el.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> > ---
> > 
> > This seems to shut up my GCC, I wonder if it is going to work gfor
> > everyone or we better add BUILD_BUG_ON(FRAME_MAXSIZE < 257) and a
> > comment and remove check.
> 
> needed a bit to get to know my old zforce driver again ;-)
> 
> 
> I may be blind, but currently I fail to see what problem the original patch 
> actually tries to fix.
> 
> buf[PAYLOAD_LENGTH] is an u8, so the max value it can contain is 255. The 
> i2c_master_recv reads buf[PAYLOAD_LENGTH]-bytes into the buffer starting at 
> buf[PAYLOAD_BODY] (= buf[2]). So it reads at max 255 bytes into a 257 byte big 
> buffer starting at index 2.
> 
> zforce_read_packet, also is an internal function used only by the interrupt 
> handler, which always only calls it with a buffer of FRAME_MAXSIZE size.
> 
> 
> The original patch said "If we get a corrupted packet with PAYLOAD_LENGTH > 
> FRAME_MAXSIZE, we will silently overwrite the stack." but payload_length can 
> never actually be greater than the buffer size?

Right, not unless we for some reason decide to adjust FRAME_MAXSIZE to
make it smaller than 257 and then fail to add the check to make sure we
do not go past the buffer.

So everything is fine now, but I guess we'd like to be more safe in the
future...

Thanks.

-- 
Dmitry
--
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/