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

[Aneesh Kumar K . V]

Jason Gunthorpe <jgg@xxxxxxxxxx> writes:

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

Yes. This even can result in error return from firmware like

[   rmm ] SMC_RMI_VERSION                   6 > RMI_RMI_ERROR_INPUT


>
> 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?
>

The goal is only to check whether the firmware function is supported,
hence the explicit check for SMCCC_RET_NOT_SUPPORTED.

	arm_smccc_1_1_invoke(smccc_dev->func_id, &res);
	ret = res.a0;

	if (ret == SMCCC_RET_NOT_SUPPORTED)
		return false;


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

I will check whether issuing an SMC64 call on 32-bit ARM is a problem.

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

IIUC, you are suggesting to do the below?

static void cca_tsm_remove(struct arm_smccc_device *sdev)
{
        tsm_report_unregister(&arm_cca_tsm_report_ops);
}

static struct arm_smccc_driver cca_tsm_driver = {
        .driver_name = "arm_cca_tsm",
        .probe = cca_tsm_probe,
        .remove = cca_tsm_remove,
        ...

-aneesh