Re: [PATCH v9 6/7] firmware: smccc: arm-cca-guest: Bind the TSM provider to an SMCCC device
From: Jason Gunthorpe <jgg@nvidia.com>
Date: 2026-08-28 19:35:18
Also in:
linux-coco, lkml
quoted hunk ↗ jump to hunk
[ ... 43 lines skipped ... ]@@ -94,6 +95,12 @@ static const struct smccc_device_info smccc_devices[] __initconst = { .requires_smc = false, .device_name = "arm-smccc-trng", }, + + { + .func_id = SMC_RSI_ABI_VERSION,
[Severity: High] Does this new entry cause uninitialized registers to be passed to the firmware? Looking at smccc_probe_smccc_device(): arm_smccc_1_1_invoke(smccc_dev->func_id, &res); The invocation only passes the func_id, leaving r1 uninitialized. Since the SMC_RSI_ABI_VERSION ABI requires r1 to hold the requested version parameter, does this leak uninitialized kernel register state to the firmware and pass a garbage ABI version? This seems like a good point.. Several other APIs had this 'pass a thing in' as part of their version contract too. There is ABI incompatabilitiy here right? It would make sense to break up the really different versions into different device strings if possible. eg v1 and v2? ... [Severity: High] Could this also execute an SMC64 call on 32-bit ARM (AArch32) systems? The smccc_devices array unconditionally includes SMC_RSI_ABI_VERSION, which is an SMC64 call. Executing an SMC64 function identifier from an AArch32 execution state is architecturally unpredictable and could cause a crash or hang on 32-bit hardware. No idea if sashiko is right , but it is what I was wondering about in the rng patch...
quoted hunk ↗ jump to hunk
[ ... 44 lines skipped ... ] +static void unregister_cca_tsm_report(void *data) +{ + tsm_report_unregister(&arm_cca_tsm_report_ops); +} + +static int cca_tsm_probe(struct arm_smccc_device *sdev) { int ret;@@ -178,30 +175,33 @@ static int __init arm_cca_guest_init(void) return -ENODEV; ret = tsm_report_register(&arm_cca_tsm_report_ops, NULL); - if (ret < 0) - pr_err("Error %d registering with TSM\n", ret); + if (ret < 0) { + dev_err_probe(&sdev->dev, ret, "Error registering with TSM\n"); + return ret; + } - return ret; + ret = devm_add_action_or_reset(&sdev->dev, unregister_cca_tsm_report, + NULL); + if (ret < 0) {
Can just make unregister the remove function. Don't need to use devm for everything. -- Jason