From: Joe Burmeister <joe.burmeister@devtank.co.uk> To: Mark Brown <broonie@kernel.org> Cc: Florian Fainelli <f.fainelli@gmail.com>, Ray Jui <rjui@broadcom.com>, Scott Branden <sbranden@broadcom.com>, bcm-kernel-feedback-list@broadcom.com, linux-spi@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, nsaenz@kernel.org Subject: Re: [PATCH] spi: bcm2835: Fix buffer overflow with CS able to go beyond limit. Date: Fri, 23 Apr 2021 15:12:11 +0100 [thread overview] Message-ID: <672e8d77-ee5c-f10f-0bd3-f8708dfc24c8@devtank.co.uk> (raw) In-Reply-To: <20210423115724.GB5507@sirena.org.uk> 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
WARNING: multiple messages have this Message-ID (diff)
From: Joe Burmeister <joe.burmeister@devtank.co.uk> To: Mark Brown <broonie@kernel.org> Cc: Florian Fainelli <f.fainelli@gmail.com>, Ray Jui <rjui@broadcom.com>, Scott Branden <sbranden@broadcom.com>, bcm-kernel-feedback-list@broadcom.com, linux-spi@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, nsaenz@kernel.org Subject: Re: [PATCH] spi: bcm2835: Fix buffer overflow with CS able to go beyond limit. Date: Fri, 23 Apr 2021 15:12:11 +0100 [thread overview] Message-ID: <672e8d77-ee5c-f10f-0bd3-f8708dfc24c8@devtank.co.uk> (raw) In-Reply-To: <20210423115724.GB5507@sirena.org.uk> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-04-23 14:12 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-20 8:34 [PATCH] spi: bcm2835: Fix buffer overflow with CS able to go beyond limit Joe Burmeister 2021-04-20 8:34 ` Joe Burmeister 2021-04-22 16:42 ` Mark Brown 2021-04-22 16:42 ` Mark Brown 2021-04-22 16:50 ` Florian Fainelli 2021-04-22 16:50 ` Florian Fainelli 2021-04-22 20:10 ` Joe Burmeister 2021-04-22 20:10 ` Joe Burmeister 2021-04-22 23:49 ` Florian Fainelli 2021-04-22 23:49 ` Florian Fainelli 2021-04-23 10:03 ` Joe Burmeister 2021-04-23 10:03 ` Joe Burmeister 2021-04-23 11:57 ` Mark Brown 2021-04-23 11:57 ` Mark Brown 2021-04-23 14:12 ` Joe Burmeister [this message] 2021-04-23 14:12 ` Joe Burmeister 2021-04-23 16:20 ` Mark Brown 2021-04-23 16:20 ` Mark Brown 2021-04-23 17:34 ` Nicolas Saenz Julienne 2021-04-23 17:34 ` Nicolas Saenz Julienne 2021-05-01 19:51 ` Lukas Wunner 2021-05-04 11:51 ` Mark Brown 2021-05-04 11:51 ` Mark Brown 2021-05-04 13:53 ` Lukas Wunner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=672e8d77-ee5c-f10f-0bd3-f8708dfc24c8@devtank.co.uk \ --to=joe.burmeister@devtank.co.uk \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=broonie@kernel.org \ --cc=f.fainelli@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rpi-kernel@lists.infradead.org \ --cc=linux-spi@vger.kernel.org \ --cc=nsaenz@kernel.org \ --cc=rjui@broadcom.com \ --cc=sbranden@broadcom.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.