Re: [RFC PATCH 1/3] dt-bindings: watchdog: arm-smc-wdt: allow a list of SMC ids

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: watchdog: arm-smc-wdt: unsafe SMC ID probing list in arm,smc-id
--

commit dea383b087f6f2623ae4ede98f366e18fd9b9ed8
Author: Mateusz Nowicki <mateusz.nowicki@posteo.net>

dt-bindings: watchdog: arm-smc-wdt: allow a list of SMC ids

This commit changes the arm,smc-id property in the arm-smc-wdt binding from a
single uint32 to a uint32-array of up to 4 items so the driver can probe a
list of candidate SMC function IDs in order across different firmware
versions.

> diff --git a/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
> index 8e9d0b7e82444..9844b23ef51ce 100644
> --- a/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
> @@ -18,9 +18,14 @@ properties:
>        - arm,smc-wdt
>  
>    arm,smc-id:
> -    $ref: /schemas/types.yaml#/definitions/uint32
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    maxItems: 4
>      description: |
> -      The ATF smc function id used by the firmware.
> +      The ATF smc function id used by the firmware, or a list of ids for
> +      firmware versions that use different ones. The ids are tried in order
> +      until the firmware accepts the init call, so only ids that are known
> +      to be this watchdog service on the platform belong in the list.
>        Defaults to 0x82003D06 if unset.

[Severity: Medium]
Is it safe to define arm,smc-id as a list of candidate SMC function IDs to be
sequentially probed in EL3/S-EL1 secure firmware?

In drivers/watchdog/arm_smc_wdt.c, smcwd_probe() invokes smcwd_call(wdd,
SMCWD_INIT, 0, &res), which issues arm_smccc_smc() with a raw function ID and
arguments a1=0, a2=0 without any UUID or version handshake:

drivers/watchdog/arm_smc_wdt.c:smcwd_probe() {
	if (of_property_read_u32(pdev->dev.of_node, "arm,smc-id",
				 &smc_func_id))
		smc_func_id = 0x82003D06;
	watchdog_set_drvdata(wdd, (void *)(uintptr_t)smc_func_id);

	err = smcwd_call(wdd, SMCWD_INIT, 0, &res);
}

Because SiP and Trusted OS SMCCC function IDs are vendor- and
firmware-specific, an ID used for the watchdog in one firmware release is not
guaranteed to be unimplemented across all other firmware implementations for
that SoC. Could probing a list of candidate IDs invoke an unrelated
privileged SMC handler on firmware versions that assign one of the earlier
IDs to another service, causing unintended secure-world side effects or
binding the watchdog driver to the wrong service if it returns 0?

Also, existing Devicetree consumers (such as older kernels calling
of_property_read_u32() on arm,smc-id) only read the first cell and will fail
if the active firmware uses a subsequent ID in the array. Since a running
firmware image implements a single watchdog SMC ID, should differences
between mutually exclusive firmware builds be handled by the bootloader
updating the Devicetree, a DT overlay, or board-specific DTS files rather
than encoding a trial-and-error probing list in the SoC dtsi?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104161553.30115-1-mateusz.nowicki@posteo.net?part=1