| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4748499f-789c-45a8-b50a-2dd09f4bac8c@notapiano>
Date: Wed, 22 May 2024 14:41:48 -0400
From: Nícolas F. R. A. Prado <nfraprado@...labora.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org,
linux-kernel@...r.kernel.org,
Neil Armstrong <neil.armstrong@...aro.org>
Subject: Re: [PATCH v1 2/2] spi: Check if transfer is mapped before calling
DMA sync APIs
On Wed, May 22, 2024 at 08:09:50PM +0300, Andy Shevchenko wrote:
> The resent update to remove the orig_nents checks revealed
> that not all DMA sync backends can cope with the unallocated
> SG list, while supplying orig_nents == 0 (the commit 861370f49ce4
> ("iommu/dma: force bouncing if the size is not cacheline-aligned"),
> for example, makes that happen for the IOMMU case). It means
> we have to check if the buffers are DMA mapped before trying
> to sync them. Re-introduce that check in a form of calling
> ->can_dma() in the same way as it's done in the DMA mapping loop
> for the SPI transfers.
>
> Reported-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> Reported-by: Neil Armstrong <neil.armstrong@...aro.org>
> Closes: https://lore.kernel.org/r/8ae675b5-fcf9-4c9b-b06a-4462f70e1322@linaro.org
> Closes: https://lore.kernel.org/all/d3679496-2e4e-4a7c-97ed-f193bd53af1d@notapiano
> Fixes: 8cc3bad9d9d6 ("spi: Remove unneded check for orig_nents")
> Suggested-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Hi Andy,
sorry I keep bothering you.
I tested this series and I still get the oops (attached at the end for
reference). When I tried this change originally, I added it on top of the
patches you had supplied. And as it turns out one of them was necessary.
Specifically, if I add
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f94420858c22..9bc9fd10d538 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1220,6 +1220,11 @@ void spi_unmap_buf(struct spi_controller *ctlr, struct device *dev,
spi_unmap_buf_attrs(ctlr, dev, sgt, dir, 0);
}
+/* Dummy SG for unidirect transfers */
+static struct scatterlist dummy_sg = {
+ .page_link = SG_END,
+};
+
static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
{
struct device *tx_dev, *rx_dev;
@@ -1258,6 +1263,8 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
attrs);
if (ret != 0)
return ret;
+ } else {
+ xfer->tx_sg.sgl = &dummy_sg;
}
if (xfer->rx_buf != NULL) {
@@ -1271,6 +1278,8 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
return ret;
}
+ } else {
+ xfer->rx_sg.sgl = &dummy_sg;
}
}
/* No transfer has been mapped, bail out with success */
on top of this series, then I don't get any oops. (The memset doesn't seem to be
required, or at least in my test it didn't trigger any issue).
I guess this is needed because the can_dma "flag" is shared between rx and tx,
but either one of them might not have a buffer assigned (for unidirectional
transfers).
Sorry about the confusion.
Thanks,
Nícolas
Oops:
[ 3.085228] Unable to handle kernel NULL pointer dereference at virtual address 000000000000001c
[ 3.096144] Unable to handle kernel NULL pointer dereference at virtual address 000000000000001c
[ 3.101468] Mem abort info:
[ 3.110363] Mem abort info:
[ 3.113239] ESR = 0x0000000096000004
[ 3.116114] ESR = 0x0000000096000004
[ 3.119969] EC = 0x25: DABT (current EL), IL = 32 bits
[ 3.123827] EC = 0x25: DABT (current EL), IL = 32 bits
[ 3.129284] SET = 0, FnV = 0
[ 3.134744] SET = 0, FnV = 0
[ 3.137881] EA = 0, S1PTW = 0
[ 3.141022] EA = 0, S1PTW = 0
[ 3.144247] FSC = 0x04: level 0 translation fault
[ 3.147475] FSC = 0x04: level 0 translation fault
[ 3.152491] Data abort info:
[ 3.157505] Data abort info:
[ 3.160468] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[ 3.163434] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[ 3.169065] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 3.169069] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 3.169073] [000000000000001c] user address but active_mm is swapper
[ 3.169078] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[ 3.174711] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 3.179896] Modules linked in:
[ 3.179903] CPU: 6 PID: 68 Comm: kworker/u32:2 Not tainted 6.9.0-next-20240515-00006-g6c6aba391be0 #427
[ 3.185352] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 3.191869] Hardware name: Google Kingoftown (DT)
[ 3.191872] Workqueue: events_unbound deferred_probe_work_func
[ 3.198309] [000000000000001c] user address but active_mm is swapper
[ 3.203495]
[ 3.203497] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 3.247739] pc : iommu_dma_sync_sg_for_device+0x28/0x100
[ 3.253204] lr : __dma_sync_sg_for_device+0x28/0x4c
[ 3.258220] sp : ffff800080942dd0
[ 3.261624] x29: ffff800080942dd0 x28: ffff1a58c1012010 x27: ffff1a58c1012010
[ 3.268953] x26: ffff1a58c31a0800 x25: ffff1a58c31a0c80 x24: 00000000000186a0
[ 3.276285] x23: ffff1a58c1012010 x22: 0000000000000001 x21: 0000000000000000
[ 3.283615] x20: ffffc3561c10c718 x19: 0000000000000000 x18: ffffc3561cde8948
[ 3.290943] x17: 0000000000010108 x16: 0000000000000000 x15: 0000000000000002
[ 3.298274] x14: ffff1a58c09156c0 x13: 0000000000000001 x12: 0000000000000000
[ 3.305602] x11: 071c71c71c71c71c x10: 0000000000000aa0 x9 : ffff800080942c90
[ 3.312932] x8 : ffff1a5a36da4800 x7 : 4000000000000000 x6 : ffff1a5a36da4828
[ 3.320265] x5 : ffffc3561b51a780 x4 : ffffc3561a704acc x3 : 0000000000000001
[ 3.327596] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1a58c1012010
[ 3.334927] Call trace:
[ 3.337442] iommu_dma_sync_sg_for_device+0x28/0x100
[ 3.342540] __dma_sync_sg_for_device+0x28/0x4c
[ 3.347203] spi_transfer_one_message+0x3a8/0x700
[ 3.352042] __spi_pump_transfer_message+0x198/0x4dc
[ 3.357143] __spi_sync+0x2a0/0x3c4
[ 3.360738] spi_sync+0x30/0x54
[ 3.363971] spi_mem_exec_op+0x26c/0x41c
[ 3.368008] spi_nor_spimem_read_data+0x148/0x158
[ 3.372848] spi_nor_read_data+0x30/0x3c
[ 3.376881] spi_nor_read_sfdp+0x74/0xe4
[ 3.380916] spi_nor_parse_sfdp+0x120/0x11d0
[ 3.385314] spi_nor_sfdp_init_params_deprecated+0x3c/0x8c
[ 3.390951] spi_nor_scan+0x7ac/0xef8
[ 3.394721] spi_nor_probe+0x94/0x2ec
[ 3.398490] spi_mem_probe+0x6c/0xac
[ 3.402171] spi_probe+0x84/0xe4
[ 3.405491] really_probe+0xbc/0x2a0
[ 3.409173] __driver_probe_device+0x78/0x12c
[ 3.413654] driver_probe_device+0x40/0x160
[ 3.417961] __device_attach_driver+0xb8/0x134
[ 3.422533] bus_for_each_drv+0x84/0xe0
[ 3.426478] __device_attach+0xa8/0x1b0
[ 3.430423] device_initial_probe+0x14/0x20
[ 3.434720] bus_probe_device+0xa8/0xac
[ 3.438664] device_add+0x590/0x750
[ 3.442257] __spi_add_device+0x138/0x208
[ 3.446378] of_register_spi_device+0x394/0x57c
[ 3.451037] spi_register_controller+0x394/0x760
[ 3.455787] qcom_qspi_probe+0x328/0x390
[ 3.459826] platform_probe+0x68/0xd8
[ 3.463595] really_probe+0xbc/0x2a0
[ 3.467277] __driver_probe_device+0x78/0x12c
[ 3.471760] driver_probe_device+0x40/0x160
[ 3.476068] __device_attach_driver+0xb8/0x134
[ 3.480641] bus_for_each_drv+0x84/0xe0
[ 3.484585] __device_attach+0xa8/0x1b0
[ 3.488530] device_initial_probe+0x14/0x20
[ 3.492838] bus_probe_device+0xa8/0xac
[ 3.496784] deferred_probe_work_func+0x88/0xc0
[ 3.501442] process_one_work+0x154/0x298
[ 3.505568] worker_thread+0x304/0x408
[ 3.509425] kthread+0x118/0x11c
[ 3.512745] ret_from_fork+0x10/0x20
[ 3.516431] Code: 2a0203f5 2a0303f6 a90363f7 aa0003f7 (b9401c20)
[ 3.522686] ---[ end trace 0000000000000000 ]---
[ 3.527428] Internal error: Oops: 0000000096000004 [#2] PREEMPT SMP
[ 3.533868] Modules linked in:
[ 3.537013] CPU: 2 PID: 102 Comm: cros_ec_spi_hig Tainted: G D 6.9.0-next-20240515-00006-g6c6aba391be0 #427
[ 3.548431] Hardware name: Google Kingoftown (DT)
[ 3.553267] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 3.560412] pc : iommu_dma_sync_sg_for_device+0x28/0x100
[ 3.565877] lr : __dma_sync_sg_for_device+0x28/0x4c
[ 3.570899] sp : ffff8000809fb990
[ 3.574312] x29: ffff8000809fb990 x28: ffff1a58c0ff8010 x27: ffff1a58c0ff8010
[ 3.581644] x26: ffff1a58c4d39800 x25: ffff1a58c4d39c80 x24: 00000000000186a0
[ 3.588976] x23: ffff1a58c0ff8010 x22: 0000000000000001 x21: 0000000000000000
[ 3.596307] x20: ffffc3561c10c3d8 x19: 0000000000000000 x18: 0000000000000000
[ 3.603639] x17: 000000040044ffff x16: 005000f2b5503510 x15: 0000000000000002
[ 3.610972] x14: 0000000000000001 x13: 0000000000162b7a x12: 0000000000000001
[ 3.618304] x11: ffff8000809fb8a0 x10: ffff1a58c1f93ff8 x9 : ffff1a58c4d39c69
[ 3.625630] x8 : ffff1a58c102db04 x7 : 0000000000000000 x6 : 0000000000000001
[ 3.632962] x5 : ffffc3561b51a780 x4 : ffffc3561a704acc x3 : 0000000000000001
[ 3.640291] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1a58c0ff8010
[ 3.647623] Call trace:
[ 3.650148] iommu_dma_sync_sg_for_device+0x28/0x100
[ 3.655249] __dma_sync_sg_for_device+0x28/0x4c
[ 3.659903] spi_transfer_one_message+0x3a8/0x700
[ 3.664746] __spi_pump_transfer_message+0x198/0x4dc
[ 3.669853] __spi_sync+0x2a0/0x3c4
[ 3.673441] spi_sync_locked+0x10/0x1c
[ 3.677299] receive_n_bytes+0xc0/0x118
[ 3.681248] do_cros_ec_pkt_xfer_spi+0x3c0/0x530
[ 3.685997] cros_ec_xfer_high_pri_work+0x20/0x34
[ 3.690835] kthread_worker_fn+0xcc/0x184
[ 3.694960] kthread+0x118/0x11c
[ 3.698282] ret_from_fork+0x10/0x20
[ 3.701964] Code: 2a0203f5 2a0303f6 a90363f7 aa0003f7 (b9401c20)
[ 3.708221] ---[ end trace 0000000000000000 ]---
Powered by blists - more mailing lists