| Message ID | 20240430182705.13019-1-mans@mansr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4756fa529b2f12b7cb8f21fe229b0f6f47190829 |
| Headers | show |
| Series | spi: fix null pointer dereference within spi_sync | expand |
Hi Mans, On Tue, 30 Apr 2024 19:27:05 +0100 Mans Rullgard <mans@mansr.com> wrote: > If spi_sync() is called with the non-empty queue and the same spi_message > is then reused, the complete callback for the message remains set while > the context is cleared, leading to a null pointer dereference when the > callback is invoked from spi_finalize_current_message(). > > With function inlining disabled, the call stack might look like this: > > _raw_spin_lock_irqsave from complete_with_flags+0x18/0x58 > complete_with_flags from spi_complete+0x8/0xc > spi_complete from spi_finalize_current_message+0xec/0x184 > spi_finalize_current_message from spi_transfer_one_message+0x2a8/0x474 > spi_transfer_one_message from __spi_pump_transfer_message+0x104/0x230 > __spi_pump_transfer_message from __spi_transfer_message_noqueue+0x30/0xc4 > __spi_transfer_message_noqueue from __spi_sync+0x204/0x248 > __spi_sync from spi_sync+0x24/0x3c > spi_sync from mcp251xfd_regmap_crc_read+0x124/0x28c [mcp251xfd] > mcp251xfd_regmap_crc_read [mcp251xfd] from _regmap_raw_read+0xf8/0x154 > _regmap_raw_read from _regmap_bus_read+0x44/0x70 > _regmap_bus_read from _regmap_read+0x60/0xd8 > _regmap_read from regmap_read+0x3c/0x5c > regmap_read from mcp251xfd_alloc_can_err_skb+0x1c/0x54 [mcp251xfd] > mcp251xfd_alloc_can_err_skb [mcp251xfd] from mcp251xfd_irq+0x194/0xe70 [mcp251xfd] > mcp251xfd_irq [mcp251xfd] from irq_thread_fn+0x1c/0x78 > irq_thread_fn from irq_thread+0x118/0x1f4 > irq_thread from kthread+0xd8/0xf4 > kthread from ret_from_fork+0x14/0x28 > > Fix this by also setting message->complete to NULL when the transfer is > complete. > > Fixes: ae7d2346dc89 ("spi: Don't use the message queue if possible in spi_sync") > > Signed-off-by: Mans Rullgard <mans@mansr.com> > --- > drivers/spi/spi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 34fca94b2b5b..ca13ca47e745 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -4521,6 +4521,7 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message) > wait_for_completion(&done); > status = message->status; > } > + message->complete = NULL; > message->context = NULL; > > return status; Nice catch. Looks good to me. Thanks. Best regards,
On Tue, 30 Apr 2024 19:27:05 +0100, Mans Rullgard wrote: > If spi_sync() is called with the non-empty queue and the same spi_message > is then reused, the complete callback for the message remains set while > the context is cleared, leading to a null pointer dereference when the > callback is invoked from spi_finalize_current_message(). > > With function inlining disabled, the call stack might look like this: > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: fix null pointer dereference within spi_sync commit: 4756fa529b2f12b7cb8f21fe229b0f6f47190829 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 34fca94b2b5b..ca13ca47e745 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -4521,6 +4521,7 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message) wait_for_completion(&done); status = message->status; } + message->complete = NULL; message->context = NULL; return status;
If spi_sync() is called with the non-empty queue and the same spi_message is then reused, the complete callback for the message remains set while the context is cleared, leading to a null pointer dereference when the callback is invoked from spi_finalize_current_message(). With function inlining disabled, the call stack might look like this: _raw_spin_lock_irqsave from complete_with_flags+0x18/0x58 complete_with_flags from spi_complete+0x8/0xc spi_complete from spi_finalize_current_message+0xec/0x184 spi_finalize_current_message from spi_transfer_one_message+0x2a8/0x474 spi_transfer_one_message from __spi_pump_transfer_message+0x104/0x230 __spi_pump_transfer_message from __spi_transfer_message_noqueue+0x30/0xc4 __spi_transfer_message_noqueue from __spi_sync+0x204/0x248 __spi_sync from spi_sync+0x24/0x3c spi_sync from mcp251xfd_regmap_crc_read+0x124/0x28c [mcp251xfd] mcp251xfd_regmap_crc_read [mcp251xfd] from _regmap_raw_read+0xf8/0x154 _regmap_raw_read from _regmap_bus_read+0x44/0x70 _regmap_bus_read from _regmap_read+0x60/0xd8 _regmap_read from regmap_read+0x3c/0x5c regmap_read from mcp251xfd_alloc_can_err_skb+0x1c/0x54 [mcp251xfd] mcp251xfd_alloc_can_err_skb [mcp251xfd] from mcp251xfd_irq+0x194/0xe70 [mcp251xfd] mcp251xfd_irq [mcp251xfd] from irq_thread_fn+0x1c/0x78 irq_thread_fn from irq_thread+0x118/0x1f4 irq_thread from kthread+0xd8/0xf4 kthread from ret_from_fork+0x14/0x28 Fix this by also setting message->complete to NULL when the transfer is complete. Fixes: ae7d2346dc89 ("spi: Don't use the message queue if possible in spi_sync") Signed-off-by: Mans Rullgard <mans@mansr.com> --- drivers/spi/spi.c | 1 + 1 file changed, 1 insertion(+)