Re: [PATCH v9 6/7] firmware: smccc: arm-cca-guest: Bind the TSM provider to an SMCCC device

[Jason Gunthorpe]

> [ ... 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...

> [ ... 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