May 2024 - Linux-stable-mirror

[PATCH AUTOSEL 5.4 1/6] selftests/bpf: Prevent client connect before server bind in test_tc_tunnel.sh

by Sasha Levin

From: "Alessandro Carminati (Red Hat)" <alessandro.carminati(a)gmail.com> [ Upstream commit f803bcf9208a2540acb4c32bdc3616673169f490 ] In some systems, the netcat server can incur in delay to start listening. When this happens, the test can randomly fail in various points. This is an example error message: # ip gre none gso # encap 192.168.1.1 to 192.168.1.2, type gre, mac none len 2000 # test basic connectivity # Ncat: Connection refused. The issue stems from a race condition between the netcat client and server. The test author had addressed this problem by implementing a sleep, which I have removed in this patch. This patch introduces a function capable of sleeping for up to two seconds. However, it can terminate the waiting period early if the port is reported to be listening. Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati(a)gmail.com> Signed-off-by: Andrii Nakryiko <andrii(a)kernel.org> Link: https://lore.kernel.org/bpf/20240314105911.213411-1-alessandro.carminati@gm… Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- tools/testing/selftests/bpf/test_tc_tunnel.sh | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/bpf/test_tc_tunnel.sh b/tools/testing/selftests/bpf/test_tc_tunnel.sh index 7c76b841b17bb..21bde60c95230 100755 --- a/tools/testing/selftests/bpf/test_tc_tunnel.sh +++ b/tools/testing/selftests/bpf/test_tc_tunnel.sh @@ -71,7 +71,6 @@ cleanup() { server_listen() { ip netns exec "${ns2}" nc "${netcat_opt}" -l -p "${port}" > "${outfile}" & server_pid=$! - sleep 0.2 } client_connect() { @@ -92,6 +91,16 @@ verify_data() { fi } +wait_for_port() { + for i in $(seq 20); do + if ip netns exec "${ns2}" ss ${2:--4}OHntl | grep -q "$1"; then + return 0 + fi + sleep 0.1 + done + return 1 +} + set -e # no arguments: automated test, run all @@ -183,6 +192,7 @@ setup # basic communication works echo "test basic connectivity" server_listen +wait_for_port ${port} ${netcat_opt} client_connect verify_data @@ -194,6 +204,7 @@ ip netns exec "${ns1}" tc filter add dev veth1 egress \ section "encap_${tuntype}_${mac}" echo "test bpf encap without decap (expect failure)" server_listen +wait_for_port ${port} ${netcat_opt} ! client_connect if [[ "$tuntype" =~ "udp" ]]; then -- 2.43.0

4 months, 3 weeks

2
6
0 0

[PATCH 1/1] intel_th: pci: Convert PCIBIOS_* return codes to errnos

by Ilpo Järvinen

intel_th_pci_activate() uses pci_{read,write}_config_dword() that return PCIBIOS_* codes. The value is returned as is to the caller. The non-errno return value is returned all the way to active_store() which then returns the value like it is an error. PCIBIOS_* return codes, however, are positive (0x8X) so the return value of the store function is treated as the length consumed from the write buffer which can confuse the userspace writer. Convert PCIBIOS_* returns code using pcibios_err_to_errno() into normal errno before returning it from intel_th_pci_activate(). Fixes: a0e7df335afd ("intel_th: Perform time resync on capture start") Cc: stable(a)vger.kernel.org Signed-off-by: Ilpo Järvinen <ilpo.jarvinen(a)linux.intel.com> --- drivers/hwtracing/intel_th/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwtracing/intel_th/pci.c b/drivers/hwtracing/intel_th/pci.c index 147d338c191e..40e2c922fbe7 100644 --- a/drivers/hwtracing/intel_th/pci.c +++ b/drivers/hwtracing/intel_th/pci.c @@ -46,7 +46,7 @@ static int intel_th_pci_activate(struct intel_th *th) if (err) dev_err(&pdev->dev, "failed to read NPKDSC register\n"); - return err; + return pcibios_err_to_errno(err); } static void intel_th_pci_deactivate(struct intel_th *th) -- 2.39.2

4 months, 3 weeks

1
0
0 0

[PATCH] drm/dp_mst: Fix all mstb marked as not probed after suspend/resume

by Wayne Lin

[Why] After supend/resume, with topology unchanged, observe that link_address_sent of all mstb are marked as false even the topology probing is done without any error. It is caused by wrongly also include "ret == 0" case as a probing failure case. [How] Remove inappropriate checking conditions. Cc: Lyude Paul <lyude(a)redhat.com> Cc: Harry Wentland <hwentlan(a)amd.com> Cc: Jani Nikula <jani.nikula(a)intel.com> Cc: stable(a)vger.kernel.org Fixes: 37dfdc55ffeb ("drm/dp_mst: Cleanup drm_dp_send_link_address() a bit") Signed-off-by: Wayne Lin <Wayne.Lin(a)amd.com> --- drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c index 7f8e1cfbe19d..68831f4e502a 100644 --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c @@ -2929,7 +2929,7 @@ static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, /* FIXME: Actually do some real error handling here */ ret = drm_dp_mst_wait_tx_reply(mstb, txmsg); - if (ret <= 0) { + if (ret < 0) { drm_err(mgr->dev, "Sending link address failed with %d\n", ret); goto out; } @@ -2981,7 +2981,7 @@ static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, mutex_unlock(&mgr->lock); out: - if (ret <= 0) + if (ret < 0) mstb->link_address_sent = false; kfree(txmsg); return ret < 0 ? ret : changed; -- 2.37.3

4 months, 3 weeks

1
0
0 0

[PATCH] mtd: rawnand: Ensure ECC configuration is propagated to upper layers

by Miquel Raynal

Until recently the "upper layer" was MTD. But following incremental reworks to bring spi-nand support and more recently generic ECC support, there is now an intermediate "generic NAND" layer that also needs to get access to some values. When using "converted" ECC engines, like the software ones, these values are already propagated correctly. But otherwise when using good old raw NAND controller drivers, we need to manually set these values ourselves at the end of the "scan" operation, once these values have been negotiated. Without this propagation, later (generic) checks like the one warning users that the ECC strength is not high enough might simply no longer work. Fixes: 8c126720fe10 ("mtd: rawnand: Use the ECC framework nand_ecc_is_strong_enough() helper") Cc: stable(a)vger.kernel.org Reported-by: Sascha Hauer <s.hauer(a)pengutronix.de> Closes: https://lore.kernel.org/all/Zhe2JtvvN1M4Ompw@pengutronix.de/ Signed-off-by: Miquel Raynal <miquel.raynal(a)bootlin.com> --- Hello Sascha, this is only compile tested, would you mind checking if that fixes your setup? Thanks, Miquèl drivers/mtd/nand/raw/nand_base.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index d7dbbd469b89..acd137dd0957 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -6301,6 +6301,7 @@ static const struct nand_ops rawnand_ops = { static int nand_scan_tail(struct nand_chip *chip) { struct mtd_info *mtd = nand_to_mtd(chip); + struct nand_device *base = &chip->base; struct nand_ecc_ctrl *ecc = &chip->ecc; int ret, i; @@ -6445,9 +6446,13 @@ static int nand_scan_tail(struct nand_chip *chip) if (!ecc->write_oob_raw) ecc->write_oob_raw = ecc->write_oob; - /* propagate ecc info to mtd_info */ + /* Propagate ECC info to the generic NAND and MTD layers */ mtd->ecc_strength = ecc->strength; + if (!base->ecc.ctx.conf.strength) + base->ecc.ctx.conf.strength = ecc->strength; mtd->ecc_step_size = ecc->size; + if (!base->ecc.ctx.conf.step_size) + base->ecc.ctx.conf.step_size = ecc->size; /* * Set the number of read / write steps for one page depending on ECC @@ -6455,6 +6460,8 @@ static int nand_scan_tail(struct nand_chip *chip) */ if (!ecc->steps) ecc->steps = mtd->writesize / ecc->size; + if (!base->ecc.ctx.nsteps) + base->ecc.ctx.nsteps = ecc->steps; if (ecc->steps * ecc->size != mtd->writesize) { WARN(1, "Invalid ECC parameters\n"); ret = -EINVAL; -- 2.40.1

4 months, 3 weeks

2
2
0 0

[PATCH v2 1/2] mtd: rawnand: Fix the nand_read_data_op() early check

by Miquel Raynal

The nand_read_data_op() operation, which only consists in DATA_IN cycles, is sadly not supported by all controllers despite being very basic. The core, for some time, supposed all drivers would support it. An improvement to this situation for supporting more constrained controller added a check to verify if the operation was supported before attempting it by running the function with the check_only boolean set first, and then possibly falling back to another (possibly slightly less optimized) alternative. An even newer addition moved that check very early and probe time, in order to perform the check only once. The content of the operation was not so important, as long as the controller driver would tell whether such operation on the NAND bus would be possible or not. In practice, no buffer was provided (no fake buffer or whatever) as it is anyway not relevant for the "check_only" condition. Unfortunately, early in the function, there is an if statement verifying that the input parameters are right for normal use, making the early check always unsuccessful. Fixes: 9f820fc0651c ("mtd: rawnand: Check the data only read pattern only once") Cc: stable(a)vger.kernel.org Reported-by: Alexander Dahl <ada(a)thorsis.com> Closes: https://lore.kernel.org/linux-mtd/20240306-shaky-bunion-d28b65ea97d7@thorsi… Reported-by: Steven Seeger <steven.seeger(a)flightsystems.net> Closes: https://lore.kernel.org/linux-mtd/DM6PR05MB4506554457CF95191A670BDEF7062@DM… Signed-off-by: Miquel Raynal <miquel.raynal(a)bootlin.com> Reviewed-by: Alexander Dahl <ada(a)thorsis.com> --- drivers/mtd/nand/raw/nand_base.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index acd137dd0957..248e654ecefd 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -2173,7 +2173,7 @@ EXPORT_SYMBOL_GPL(nand_reset_op); int nand_read_data_op(struct nand_chip *chip, void *buf, unsigned int len, bool force_8bit, bool check_only) { - if (!len || !buf) + if (!len || (!check_only && !buf)) return -EINVAL; if (nand_has_exec_op(chip)) { -- 2.40.1

4 months, 3 weeks

1
1
0 0

[PATCH v2 2/2] mtd: rawnand: Bypass a couple of sanity checks during NAND identification

by Miquel Raynal

Early during NAND identification, mtd_info fields have not yet been initialized (namely, writesize and oobsize) and thus cannot be used for sanity checks yet. Of course if there is a misuse of nand_change_read_column_op() so early we won't be warned, but there is anyway no actual check to perform at this stage as we do not yet know the NAND geometry. So, if the fields are empty, especially mtd->writesize which is *always* set quite rapidly after identification, let's skip the sanity checks. nand_change_read_column_op() is subject to be used early for ONFI/JEDEC identification in the very unlikely case of: - bitflips appearing in the parameter page, - the controller driver not supporting simple DATA_IN cycles. As nand_change_read_column_op() uses nand_fill_column_cycles() the logic explaind above also applies in this secondary helper. Fixes: c27842e7e11f ("mtd: rawnand: onfi: Adapt the parameter page read to constraint controllers") Fixes: daca31765e8b ("mtd: rawnand: jedec: Adapt the parameter page read to constraint controllers") Cc: stable(a)vger.kernel.org Reported-by: Alexander Dahl <ada(a)thorsis.com> Closes: https://lore.kernel.org/linux-mtd/20240306-shaky-bunion-d28b65ea97d7@thorsi… Reported-by: Steven Seeger <steven.seeger(a)flightsystems.net> Closes: https://lore.kernel.org/linux-mtd/DM6PR05MB4506554457CF95191A670BDEF7062@DM… Signed-off-by: Miquel Raynal <miquel.raynal(a)bootlin.com> --- Changes in v2: * Dropped the double (( )) * Fixed nand_fill_column_cycles() as well. --- drivers/mtd/nand/raw/nand_base.c | 57 ++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 248e654ecefd..53e16d39af4b 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -1093,28 +1093,32 @@ static int nand_fill_column_cycles(struct nand_chip *chip, u8 *addrs, unsigned int offset_in_page) { struct mtd_info *mtd = nand_to_mtd(chip); + bool ident_stage = !mtd->writesize; - /* Make sure the offset is less than the actual page size. */ - if (offset_in_page > mtd->writesize + mtd->oobsize) - return -EINVAL; - - /* - * On small page NANDs, there's a dedicated command to access the OOB - * area, and the column address is relative to the start of the OOB - * area, not the start of the page. Asjust the address accordingly. - */ - if (mtd->writesize <= 512 && offset_in_page >= mtd->writesize) - offset_in_page -= mtd->writesize; - - /* - * The offset in page is expressed in bytes, if the NAND bus is 16-bit - * wide, then it must be divided by 2. - */ - if (chip->options & NAND_BUSWIDTH_16) { - if (WARN_ON(offset_in_page % 2)) + /* Bypass all checks during NAND identification */ + if (likely(!ident_stage)) { + /* Make sure the offset is less than the actual page size. */ + if (offset_in_page > mtd->writesize + mtd->oobsize) return -EINVAL; - offset_in_page /= 2; + /* + * On small page NANDs, there's a dedicated command to access the OOB + * area, and the column address is relative to the start of the OOB + * area, not the start of the page. Asjust the address accordingly. + */ + if (mtd->writesize <= 512 && offset_in_page >= mtd->writesize) + offset_in_page -= mtd->writesize; + + /* + * The offset in page is expressed in bytes, if the NAND bus is 16-bit + * wide, then it must be divided by 2. + */ + if (chip->options & NAND_BUSWIDTH_16) { + if (WARN_ON(offset_in_page % 2)) + return -EINVAL; + + offset_in_page /= 2; + } } addrs[0] = offset_in_page; @@ -1123,7 +1127,7 @@ static int nand_fill_column_cycles(struct nand_chip *chip, u8 *addrs, * Small page NANDs use 1 cycle for the columns, while large page NANDs * need 2 */ - if (mtd->writesize <= 512) + if (!ident_stage && mtd->writesize <= 512) return 1; addrs[1] = offset_in_page >> 8; @@ -1436,16 +1440,19 @@ int nand_change_read_column_op(struct nand_chip *chip, unsigned int len, bool force_8bit) { struct mtd_info *mtd = nand_to_mtd(chip); + bool ident_stage = !mtd->writesize; if (len && !buf) return -EINVAL; - if (offset_in_page + len > mtd->writesize + mtd->oobsize) - return -EINVAL; + if (!ident_stage) { + if (offset_in_page + len > mtd->writesize + mtd->oobsize) + return -EINVAL; - /* Small page NANDs do not support column change. */ - if (mtd->writesize <= 512) - return -ENOTSUPP; + /* Small page NANDs do not support column change. */ + if (mtd->writesize <= 512) + return -ENOTSUPP; + } if (nand_has_exec_op(chip)) { const struct nand_interface_config *conf = -- 2.40.1

4 months, 3 weeks

2
3
0 0

[PATCH v2] mtd: rawnand: rockchip: ensure NVDDR timings are rejected

by Val Packett

.setup_interface first gets called with a "target" value of NAND_DATA_IFACE_CHECK_ONLY, in which case an error is expected if the controller driver does not support the timing mode (NVDDR). Fixes: a9ecc8c814e9 ("mtd: rawnand: Choose the best timings, NV-DDR included") Signed-off-by: Val Packett <val(a)packett.cool> Cc: stable(a)vger.kernel.org --- drivers/mtd/nand/raw/rockchip-nand-controller.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c index 7baaef69d..555804476 100644 --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c @@ -420,13 +420,13 @@ static int rk_nfc_setup_interface(struct nand_chip *chip, int target, u32 rate, tc2rw, trwpw, trw2c; u32 temp; - if (target < 0) - return 0; - timings = nand_get_sdr_timings(conf); if (IS_ERR(timings)) return -EOPNOTSUPP; + if (target < 0) + return 0; + if (IS_ERR(nfc->nfc_clk)) rate = clk_get_rate(nfc->ahb_clk); else -- 2.45.0

4 months, 3 weeks

2
1
0 0

[PATCH] ACPI: APEI: EINJ: Fix einj_dev release leak

by Dan Williams

The platform driver conversion of EINJ mistakenly used platform_device_del() to unwind platform_device_register_full() at module exit. This leads to a small leak of one 'struct platform_device' instance per module load/unload cycle. Switch to platform_device_unregister() which performs both device_del() and final put_device(). Fixes: 5621fafaac00 ("EINJ: Migrate to a platform driver") Cc: <stable(a)vger.kernel.org> Cc: Jonathan Cameron <Jonathan.Cameron(a)huawei.com> Cc: Ben Cheatham <Benjamin.Cheatham(a)amd.com> Signed-off-by: Dan Williams <dan.j.williams(a)intel.com> --- drivers/acpi/apei/einj-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c index 01faca3a238a..bb9f8475ce59 100644 --- a/drivers/acpi/apei/einj-core.c +++ b/drivers/acpi/apei/einj-core.c @@ -903,7 +903,7 @@ static void __exit einj_exit(void) if (einj_initialized) platform_driver_unregister(&einj_driver); - platform_device_del(einj_dev); + platform_device_unregister(einj_dev); } module_init(einj_init);

4 months, 3 weeks

3
2
0 0

[git:media_stage/fixes] media: mgb4: Fix double debugfs remove

by Hans Verkuil

This is an automatic generated email to let you know that the following patch were queued: Subject: media: mgb4: Fix double debugfs remove Author: Martin Tůma <martin.tuma(a)digiteqautomotive.com> Date: Tue May 21 18:22:54 2024 +0200 Fixes an error where debugfs_remove_recursive() is called first on a parent directory and then again on a child which causes a kernel panic. Signed-off-by: Martin Tůma <martin.tuma(a)digiteqautomotive.com> Signed-off-by: Hans Verkuil <hverkuil-cisco(a)xs4all.nl> Fixes: 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver") Cc: <stable(a)vger.kernel.org> [hverkuil: added Fixes/Cc tags] drivers/media/pci/mgb4/mgb4_core.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) --- diff --git a/drivers/media/pci/mgb4/mgb4_core.c b/drivers/media/pci/mgb4/mgb4_core.c index 60498a5abebf..ab4f07e2e560 100644 --- a/drivers/media/pci/mgb4/mgb4_core.c +++ b/drivers/media/pci/mgb4/mgb4_core.c @@ -642,9 +642,6 @@ static void mgb4_remove(struct pci_dev *pdev) struct mgb4_dev *mgbdev = pci_get_drvdata(pdev); int i; -#ifdef CONFIG_DEBUG_FS - debugfs_remove_recursive(mgbdev->debugfs); -#endif #if IS_REACHABLE(CONFIG_HWMON) hwmon_device_unregister(mgbdev->hwmon_dev); #endif @@ -659,6 +656,10 @@ static void mgb4_remove(struct pci_dev *pdev) if (mgbdev->vin[i]) mgb4_vin_free(mgbdev->vin[i]); +#ifdef CONFIG_DEBUG_FS + debugfs_remove_recursive(mgbdev->debugfs); +#endif + device_remove_groups(&mgbdev->pdev->dev, mgb4_pci_groups); free_spi(mgbdev); free_i2c(mgbdev);

4 months, 3 weeks

1
0
0 0

[PATCH] xhci: Handle TD clearing for multiple streams case

by Hector Martin

When multiple streams are in use, multiple TDs might be in flight when an endpoint is stopped. We need to issue a Set TR Dequeue Pointer for each, to ensure everything is reset properly and the caches cleared. Change the logic so that any N>1 TDs found active for different streams are deferred until after the first one is processed, calling xhci_invalidate_cancelled_tds() again from xhci_handle_cmd_set_deq() to queue another command until we are done with all of them. Also change the error/"should never happen" paths to ensure we at least clear any affected TDs, even if we can't issue a command to clear the hardware cache, and complain loudly with an xhci_warn() if this ever happens. This problem case dates back to commit e9df17eb1408 ("USB: xhci: Correct assumptions about number of rings per endpoint.") early on in the XHCI driver's life, when stream support was first added. At that point, this condition would cause TDs to not be given back at all, causing hanging transfers - but no security bug. It was then identified but not fixed nor made into a warning in commit 674f8438c121 ("xhci: split handling halted endpoints into two steps"), which added a FIXME comment for the problem case (without materially changing the behavior as far as I can tell, though the new logic made the problem more obvious). Then later, in commit 94f339147fc3 ("xhci: Fix failure to give back some cached cancelled URBs."), it was acknowledged again. This commit was unfortunately not reviewed at all, as it was authored by the maintainer directly. Had it been, perhaps a second set of eyes would've noticed that it does not fix the bug, but rather just makes it (much) worse. It turns the "transfers hang" bug into a "random memory corruption" bug, by blindly marking TDs as complete without actually clearing them at all nor moving the dequeue pointer past them, which means they aren't actually complete, and the xHC will try to transfer data to/from them when the endpoint resumes, now to freed memory buffers. This could have been a legitimate oversight, but apparently the commit author was aware of the problem (yet still chose to submit it): It was still mentioned as a FIXME, an xhci_dbg() was added to log the problem condition, and the remaining issue was mentioned in the commit description. The choice of making the log type xhci_dbg() for what is, at this point, a completely unhandled and known broken condition is puzzling and unfortunate, as it guarantees that no actual users would see the log in production, thereby making it nigh undebuggable (indeed, even if you turn on DEBUG, the message doesn't really hint at there being a problem at all). It took me *months* of random xHC crashes to finally find a reliable repro and be able to do a deep dive debug session, which could all have been avoided had this unhandled, broken condition been actually reported with a warning, as it should have been as a bug intentionally left in unfixed (never mind that it shouldn't have been left in at all). > Another fix to solve clearing the caches of all stream rings with > cancelled TDs is needed, but not as urgent. 3 years after that statement and 14 years after the original bug was introduced, I think it's finally time to fix it. And maybe next time let's not leave bugs unfixed (that are actually worse than the original bug), and let's actually get people to review kernel commits please. Fixes xHC crashes and IOMMU faults with UAS devices when handling errors/faults. Easiest repro is to use `hdparm` to mark an early sector (e.g. 1024) on a disk as bad, then `cat /dev/sdX > /dev/null` in a loop. At least in the case of JMicron controllers, the read errors end up having to cancel two TDs (for two queued requests to different streams) and the one that didn't get cleared properly ends up faulting the xHC entirely when it tries to access DMA pages that have since been unmapped, referred to by the stale TDs. This normally happens quickly (after two or three loops). After this fix, I left the `cat` in a loop running overnight and experienced no xHC failures, with all read errors recovered properly. Repro'd and tested on an Apple M1 Mac Mini (dwc3 host). On systems without an IOMMU, this bug would instead silently corrupt freed memory, making this a security bug (even on systems with IOMMUs this could silently corrupt memory belonging to other USB devices on the same controller, so it's still a security bug). Given that the kernel autoprobes partition tables, I'm pretty sure a malicious USB device pretending to be a UAS device and reporting an error with the right timing could deliberately trigger a UAF and write to freed memory, with no user action. Fixes: e9df17eb1408 ("USB: xhci: Correct assumptions about number of rings per endpoint.") Fixes: 94f339147fc3 ("xhci: Fix failure to give back some cached cancelled URBs.") Fixes: 674f8438c121 ("xhci: split handling halted endpoints into two steps") Cc: stable(a)vger.kernel.org Cc: security(a)kernel.org Signed-off-by: Hector Martin <marcan(a)marcan.st> --- drivers/usb/host/xhci-ring.c | 54 +++++++++++++++++++++++++++++++++++--------- drivers/usb/host/xhci.h | 1 + 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 575f0fd9c9f1..9c06502be098 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -1034,13 +1034,27 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) break; case TD_DIRTY: /* TD is cached, clear it */ case TD_HALTED: + case TD_CLEARING_CACHE_DEFERRED: + if (cached_td) { + if (cached_td->urb->stream_id != td->urb->stream_id) { + /* Multiple streams case, defer move dq */ + xhci_dbg(xhci, + "Move dq deferred: stream %u URB %p\n", + td->urb->stream_id, td->urb); + td->cancel_status = TD_CLEARING_CACHE_DEFERRED; + break; + } + + /* Should never happen, at least try to clear the TD if it does */ + xhci_warn(xhci, + "Found multiple active URBs %p and %p in stream %u?\n", + td->urb, cached_td->urb, + td->urb->stream_id); + td_to_noop(xhci, ring, cached_td, false); + cached_td->cancel_status = TD_CLEARED; + } + td->cancel_status = TD_CLEARING_CACHE; - if (cached_td) - /* FIXME stream case, several stopped rings */ - xhci_dbg(xhci, - "Move dq past stream %u URB %p instead of stream %u URB %p\n", - td->urb->stream_id, td->urb, - cached_td->urb->stream_id, cached_td->urb); cached_td = td; break; } @@ -1060,10 +1074,16 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) if (err) { /* Failed to move past cached td, just set cached TDs to no-op */ list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) { - if (td->cancel_status != TD_CLEARING_CACHE) + /* + * Deferred TDs need to have the deq pointer set after the above command + * completes, so if that failed we just give up on all of them (and + * complain loudly since this could cause issues due to caching). + */ + if (td->cancel_status != TD_CLEARING_CACHE && + td->cancel_status != TD_CLEARING_CACHE_DEFERRED) continue; - xhci_dbg(xhci, "Failed to clear cancelled cached URB %p, mark clear anyway\n", - td->urb); + xhci_warn(xhci, "Failed to clear cancelled cached URB %p, mark clear anyway\n", + td->urb); td_to_noop(xhci, ring, td, false); td->cancel_status = TD_CLEARED; } @@ -1350,6 +1370,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, struct xhci_ep_ctx *ep_ctx; struct xhci_slot_ctx *slot_ctx; struct xhci_td *td, *tmp_td; + bool deferred = false; ep_index = TRB_TO_EP_INDEX(le32_to_cpu(trb->generic.field[3])); stream_id = TRB_TO_STREAM_ID(le32_to_cpu(trb->generic.field[2])); @@ -1436,6 +1457,8 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, xhci_dbg(ep->xhci, "%s: Giveback cancelled URB %p TD\n", __func__, td->urb); xhci_td_cleanup(ep->xhci, td, ep_ring, td->status); + } else if (td->cancel_status == TD_CLEARING_CACHE_DEFERRED) { + deferred = true; } else { xhci_dbg(ep->xhci, "%s: Keep cancelled URB %p TD as cancel_status is %d\n", __func__, td->urb, td->cancel_status); @@ -1445,8 +1468,17 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, ep->ep_state &= ~SET_DEQ_PENDING; ep->queued_deq_seg = NULL; ep->queued_deq_ptr = NULL; - /* Restart any rings with pending URBs */ - ring_doorbell_for_active_rings(xhci, slot_id, ep_index); + + if (deferred) { + /* We have more streams to clear */ + xhci_dbg(ep->xhci, "%s: Pending TDs to clear, continuing with invalidation\n", + __func__); + xhci_invalidate_cancelled_tds(ep); + } else { + /* Restart any rings with pending URBs */ + xhci_dbg(ep->xhci, "%s: All TDs cleared, ring doorbell\n", __func__); + ring_doorbell_for_active_rings(xhci, slot_id, ep_index); + } } static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id, diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 6f4bf98a6282..aa4379bdb90c 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1276,6 +1276,7 @@ enum xhci_cancelled_td_status { TD_DIRTY = 0, TD_HALTED, TD_CLEARING_CACHE, + TD_CLEARING_CACHE_DEFERRED, TD_CLEARED, }; --- base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6 change-id: 20240524-xhci-streams-124e88db52e6 Best regards, -- Hector Martin <marcan(a)marcan.st>

4 months, 3 weeks

3
2
0 0

2024

2023

2022

2021

2020

2019

2018

2017

Linux-stable-mirror May 2024