Re: [PATCH] spi: bcm2835: Fix buffer overflow with CS able to go beyond limit.

[Joe Burmeister <joe.burmeister@xxxxxxxxxxxxx>]

On 23/04/2021 12:57, Mark Brown wrote:
> On Fri, Apr 23, 2021 at 11:03:22AM +0100, Joe Burmeister wrote:
>> On 23/04/2021 00:49, Florian Fainelli wrote:
>>> Right, which means that we should probably seek a solution within the
>>> SPI core itself, even if you can only test with spi-bcm2835.c chances
>>> are that the fix would be applicable for other controllers if done in
>>> the core.
>> I'm not sure it's possible to do in the core alone. The numb of the
>> issue is the core changes ctlr->num_chipselect to what is in the device
>> tree and some drivers are cool with that overs quietly stomp memory.
> I wouldn't expect any controller to be OK with that?  Drivers can store
> per-client data in spi_device->controller_data which doesn't need
> scaling (but is also not so helpful if you need to look at clients other
> than the one you're currently controlling).
I can see a number which certainly wouldn't. Though I don't want to
assume that all don't.

If we are happy just not letting the core expand num_chipselect that
does stop the condition on everything.

Any controller that can go higher without issue could them have their
num_chipselect set to what their real limit is if this enforcement
causes an issue.


>> I've got a simple little patch to warn when the core expands
>> ctlr->num_chipselect. This warning won't go off in bcm2835 with my patch
>> because I am also extending ctlr->num_chipselect to the amount in the
>> device tree before the core does that expansion. Hopefully that new
>> warning would make people investigate and fix problem drivers.
>>>> There is protection in spi_add_device, which will catch extra added
>>>> later, but not ones in the device tree when the spi controller was
>>>> registered.
>>> Not sure I follow you, if we have the overlay before
>>> spi_register_controller() is called, how can the check there not
>>> trigger? And if we load the overlay later when the SPI controller is
>>> already registered, why does not spi_add_device()'s check work?
>> I think it might be a RPI thing. I think it is merging in the overlay
>> and giving Linux one already merged.
> If the overlay is handled by the bootloader then from the point of view
> of Linux there is no overlay - sounds like there's an issue in the
> overlay, it should be overriding something that it doesn't?
Does it matter if the final device tree was compiled like that in the
first place or merge into that by the bootloader?

The limit isn't an hardware issue because the bcm2835 just uses any
gpios for CS. So hardware like ours with 8 spi chips on the bus is fine.
The driver's limit at 4 is arbitrary.

My patch for the bcm2835 just compares of what is in the device
tree and the harcoded limit and uses the largest. Other drivers do this.

Of course we could just raise BCM2835_SPI_NUM_CS to 8 or more if that is
preferred. Does seams like the dynamic solution is less favoured.

Regards,

Joe