Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1193511 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] Input: zforce_ts - fix playload length check Heiko Stübner <heiko.stuebner@bq.com> - 2015-07-27 23:40 +0200
    Re: [PATCH] Input: zforce_ts - fix playload length check Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-07-27 23:50 +0200
      Re: [PATCH] Input: zforce_ts - fix playload length check Heiko Stübner <heiko.stuebner@bq.com> - 2015-07-28 00:00 +0200
        Re: [PATCH] Input: zforce_ts - fix playload length check Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-07-28 00:20 +0200

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

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?


Very confused
Heiko

>  drivers/input/touchscreen/zforce_ts.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/zforce_ts.c
> b/drivers/input/touchscreen/zforce_ts.c index 2554efd..542ff02 100644
> --- a/drivers/input/touchscreen/zforce_ts.c
> +++ b/drivers/input/touchscreen/zforce_ts.c
> @@ -441,7 +441,9 @@ static int zforce_read_packet(struct zforce_ts *ts, u8
> *buf) goto unlock;
>  	}
> 
> -	if (buf[PAYLOAD_LENGTH] == 0 || buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE) {
> +	if (buf[PAYLOAD_LENGTH] == 0 ||
> +	    (FRAME_MAXSIZE - 2 < 255 &&
> +	     buf[PAYLOAD_LENGTH] > FRAME_MAXSIZE - 2)) {
>  		dev_err(&client->dev, "invalid payload length: %d\n",
>  			buf[PAYLOAD_LENGTH]);
>  		ret = -EIO;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1193516

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...

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1193518

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) actually 
only checks against kernel-developer making errors in the future and not 
something the hardware can cause.


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>


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1193531

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web