Re: [PATCH] Input: zforce_ts - fix playload length check

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Mon, Jul 27, 2015 at 11:53:27PM +0200, Heiko Stübner wrote:
> Am Montag, 27. Juli 2015, 14:44:42 schrieb Dmitry Torokhov:
> > 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@intel.com>
> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.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...
> 
> I would argue that FRAME_MAXSIZE already indicates that it should not be 
> changed. It's the maximum size a single frame can be. And this size is a 
> property of the hardware itself, because of the format, 257 bytes is always 
> the maximum you could get (2 bytes header + at max 255 bytes payload).
> 
> So this second check (while only taking up a minimal amount of time)

It does not take any time as it gets optimized out completely (with
current FRAME_MAXSIZE value).

> actually 
> only checks against kernel-developer making errors in the future and not 
> something the hardware can cause.

Right.

> 
> 
> But your change itself looks correct, so if you prefer to keep that check you 
> can also add my
> Reviewed-by: Heiko Stuebner <heiko.stuebner@bq.com>

I guess I'll sit on it. Another option is to revert the original change
and be done with it.

Thanks.

-- 
Dmitry