Re: [PATCH v2 1/2] sc16is7xx: Prevent TX buffer overrun, prevent crash

Re: [PATCH v2 1/2] sc16is7xx: Prevent TX buffer overrun, prevent crash

[Jakub Kiciński]

On Wed, 30 Sep 2015 16:09:44 +0200, Florian Achleitner wrote:
> If the chip wrongly reports a TX FIFO space, bigger than the driver's
> buffer, it runs over and destroys the struct sc16is7xx_port, its
> struct kworker, and very likely a lot more.
> For us, this lead to the immediate crash of the driver's kworker thread.
> 
> Prevent a buffer overrun by adding a length check.
> 
> Signed-off-by: Florian Achleitner <achleitner.florian-JpyZg0bqyNJBDgjK7y7TUQ@public.gmane.org>

Hi Florian!

Thanks a lot for the submission.  Would you mind digging a little
deeper into this problem?  I think the root cause is that the SPI
read fails for some reason and our driver does not handle that
properly, i.e. we don't read 255, it's just a random/failure value.
Unfortunatelly I don't have any boards with this chip any more to
do any tests or development myself, so it's on you ;)

This driver initially supported only I2C and in I2C regmap code if
the read fails we will always get a zero value.  Therefore we felt
free to ignore in sc16is7xx_port_read() the return value of
regmap_read().

Could you please run your tests again and see if perhaps the read is
failing?  In that case we should zero the return value from
sc16is7xx_port_read().

I think from this thread: 
https://lkml.org/lkml/2008/2/20/271
we can assume that zero-length writes are a valid use-case for SPI 
and if so could you please test the driver for your SPI controller?
Perhaps the zero-length check should be placed in the SPI controller
driver?

I am OK with adding the sanity checks but lets first get to the bottom
of this!

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sc16is7xx: Prevent TX buffer overrun, prevent crash

[Jakub Kiciński]

On Wed, 30 Sep 2015 16:09:44 +0200, Florian Achleitner wrote:
> If the chip wrongly reports a TX FIFO space, bigger than the driver's
> buffer, it runs over and destroys the struct sc16is7xx_port, its
> struct kworker, and very likely a lot more.
> For us, this lead to the immediate crash of the driver's kworker thread.
> 
> Prevent a buffer overrun by adding a length check.
> 
> Signed-off-by: Florian Achleitner <achleitner.florian-JpyZg0bqyNJBDgjK7y7TUQ@public.gmane.org>

Hi Florian!

Thanks a lot for the submission.  Would you mind digging a little
deeper into this problem?  I think the root cause is that the SPI
read fails for some reason and our driver does not handle that
properly, i.e. we don't read 255, it's just a random/failure value.
Unfortunatelly I don't have any boards with this chip any more to
do any tests or development myself, so it's on you ;)

This driver initially supported only I2C and in I2C regmap code if
the read fails we will always get a zero value.  Therefore we felt
free to ignore in sc16is7xx_port_read() the return value of
regmap_read().

Could you please run your tests again and see if perhaps the read is
failing?  In that case we should zero the return value from
sc16is7xx_port_read().

I think from this thread: 
https://lkml.org/lkml/2008/2/20/271
we can assume that zero-length writes are a valid use-case for SPI 
and if so could you please test the driver for your SPI controller?
Perhaps the zero-length check should be placed in the SPI controller
driver?

I am OK with adding the sanity checks but lets first get to the bottom
of this!

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sc16is7xx: Prevent TX buffer overrun, prevent crash

[Florian Achleitner]

Hi Jakub!

On Saturday 03 October 2015 19:52:57 Jakub Kiciński wrote:
> Hi Florian!
> 
> Thanks a lot for the submission.  Would you mind digging a little
> deeper into this problem?  I think the root cause is that the SPI
> read fails for some reason and our driver does not handle that
> properly, i.e. we don't read 255, it's just a random/failure value.
> Unfortunatelly I don't have any boards with this chip any more to
> do any tests or development myself, so it's on you ;)

No problem, the board is still on the desk :)

> This driver initially supported only I2C and in I2C regmap code if
> the read fails we will always get a zero value.  Therefore we felt
> free to ignore in sc16is7xx_port_read() the return value of
> regmap_read().
> 
> Could you please run your tests again and see if perhaps the read is
> failing?  In that case we should zero the return value from
> sc16is7xx_port_read().

I added a dev_err in *_port_read,write,update and *_fifo_read,write. No regmap 
function returned an error in my test, but the 255 read value is still there.

> I think from this thread:
> https://lkml.org/lkml/2008/2/20/271
> we can assume that zero-length writes are a valid use-case for SPI
> and if so could you please test the driver for your SPI controller?
> Perhaps the zero-length check should be placed in the SPI controller
> driver?

I digged a little deeper and did some measurements to support my idea.
I think the reason for the 255 read is that the chip does not support the zero 
length write. 

The chip's SPI interface defines two sorts of frames. One for normal register 
access, which is essentially an address followed by one byte of data, either 
read or written.

The second type is for accessing the fifo. It has an address and two bytes of 
data, by definition. 

If the master now issues a zero length write, it sends only the address byte, 
but the chip will expect two following data bytes, which do not arrive. 
Instead it will consume the following frame. When this frame is the tx fifo 
level read, the chip will not drive its SO line (still expecting a fifo 
write), and the master reads 255. Now two bytes were clocked, and they are 
back in sync. However, the value is crap.

If my theorie is true, we would also have to make sure, that fifo access is 
always two bytes to keep it synced. I will check this, and craft another 
patch, if neccessary.

> 
> I am OK with adding the sanity checks but lets first get to the bottom
> of this!

I agree. I split the patch in two parts, because I would vote for patch 1/2 
unconditionally, to prevent buffer overruns, whatever happens. 
And then, use patch 2/2 only if it is the right solution.
> 
> Thanks!

Thanks,
Florian

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sc16is7xx: Prevent TX buffer overrun, prevent crash

[Florian Achleitner]

Hi Jakub!

On Saturday 03 October 2015 19:52:57 Jakub Kiciński wrote:
> Hi Florian!
> 
> Thanks a lot for the submission.  Would you mind digging a little
> deeper into this problem?  I think the root cause is that the SPI
> read fails for some reason and our driver does not handle that
> properly, i.e. we don't read 255, it's just a random/failure value.
> Unfortunatelly I don't have any boards with this chip any more to
> do any tests or development myself, so it's on you ;)

No problem, the board is still on the desk :)

> This driver initially supported only I2C and in I2C regmap code if
> the read fails we will always get a zero value.  Therefore we felt
> free to ignore in sc16is7xx_port_read() the return value of
> regmap_read().
> 
> Could you please run your tests again and see if perhaps the read is
> failing?  In that case we should zero the return value from
> sc16is7xx_port_read().

I added a dev_err in *_port_read,write,update and *_fifo_read,write. No regmap 
function returned an error in my test, but the 255 read value is still there.

> I think from this thread:
> https://lkml.org/lkml/2008/2/20/271
> we can assume that zero-length writes are a valid use-case for SPI
> and if so could you please test the driver for your SPI controller?
> Perhaps the zero-length check should be placed in the SPI controller
> driver?

I digged a little deeper and did some measurements to support my idea.
I think the reason for the 255 read is that the chip does not support the zero 
length write. 

The chip's SPI interface defines two sorts of frames. One for normal register 
access, which is essentially an address followed by one byte of data, either 
read or written.

The second type is for accessing the fifo. It has an address and two bytes of 
data, by definition. 

If the master now issues a zero length write, it sends only the address byte, 
but the chip will expect two following data bytes, which do not arrive. 
Instead it will consume the following frame. When this frame is the tx fifo 
level read, the chip will not drive its SO line (still expecting a fifo 
write), and the master reads 255. Now two bytes were clocked, and they are 
back in sync. However, the value is crap.

If my theorie is true, we would also have to make sure, that fifo access is 
always two bytes to keep it synced. I will check this, and craft another 
patch, if neccessary.

> 
> I am OK with adding the sanity checks but lets first get to the bottom
> of this!

I agree. I split the patch in two parts, because I would vote for patch 1/2 
unconditionally, to prevent buffer overruns, whatever happens. 
And then, use patch 2/2 only if it is the right solution.
> 
> Thanks!

Thanks,
Florian

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sc16is7xx: Prevent TX buffer overrun, prevent crash

[Florian Achleitner]

Hi,

some news.
On Monday 05 October 2015 14:34:01 Florian Achleitner wrote:
> I digged a little deeper and did some measurements to support my idea.
> I think the reason for the 255 read is that the chip does not support the
> zero length write.
> 
> The chip's SPI interface defines two sorts of frames. One for normal
> register access, which is essentially an address followed by one byte of
> data, either read or written.
> 
> The second type is for accessing the fifo. It has an address and two bytes
> of data, by definition.
> 
> If the master now issues a zero length write, it sends only the address
> byte, but the chip will expect two following data bytes, which do not
> arrive. Instead it will consume the following frame. When this frame is the
> tx fifo level read, the chip will not drive its SO line (still expecting a
> fifo write), and the master reads 255. Now two bytes were clocked, and they
> are back in sync. However, the value is crap.
> 
> If my theorie is true, we would also have to make sure, that fifo access is
> always two bytes to keep it synced. I will check this, and craft another
> patch, if neccessary.

My theorie is wrong with fifo frame lengths. Actually, they can be be of 
arbitrary length and the SPI chip select (CS) terminates a frame. Thus, the 
complete fifo can be filled at once, and single bytes can be written, as well.

However, a zero-length write seems to confuse the chip. If I prevent it, 
everything works.

As already mentioned, all regmap operations return success.

Florian

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sc16is7xx: Prevent TX buffer overrun, prevent crash

[Florian Achleitner]

Hi,

some news.
On Monday 05 October 2015 14:34:01 Florian Achleitner wrote:
> I digged a little deeper and did some measurements to support my idea.
> I think the reason for the 255 read is that the chip does not support the
> zero length write.
> 
> The chip's SPI interface defines two sorts of frames. One for normal
> register access, which is essentially an address followed by one byte of
> data, either read or written.
> 
> The second type is for accessing the fifo. It has an address and two bytes
> of data, by definition.
> 
> If the master now issues a zero length write, it sends only the address
> byte, but the chip will expect two following data bytes, which do not
> arrive. Instead it will consume the following frame. When this frame is the
> tx fifo level read, the chip will not drive its SO line (still expecting a
> fifo write), and the master reads 255. Now two bytes were clocked, and they
> are back in sync. However, the value is crap.
> 
> If my theorie is true, we would also have to make sure, that fifo access is
> always two bytes to keep it synced. I will check this, and craft another
> patch, if neccessary.

My theorie is wrong with fifo frame lengths. Actually, they can be be of 
arbitrary length and the SPI chip select (CS) terminates a frame. Thus, the 
complete fifo can be filled at once, and single bytes can be written, as well.

However, a zero-length write seems to confuse the chip. If I prevent it, 
everything works.

As already mentioned, all regmap operations return success.

Florian

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html