Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot

Tom Rini
2023-10-25T14:57:45Z

[-- Attachment #1: Type: text/plain, Size: 4911 bytes --]

On Fri, Oct 20, 2023 at 12:57:47PM +0100, Abdellatif El Khlifi wrote:
> Hi Tom,
>
> > ________________________________________________________________________________________________________
> > *** CID 464361: Control flow issues (DEADCODE)
> > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log()
> > 142
> > 143 if (ffa_id < FFA_FIRST_ID || ffa_id > FFA_LAST_ID)
> > 144 return -EINVAL;
> > 145
> > 146 abi_idx = FFA_ID_TO_ERRMAP_ID(ffa_id);
> > 147 if (abi_idx < 0 || abi_idx >= FFA_ERRMAP_COUNT)
> > >>> CID 464361: Control flow issues (DEADCODE)
> > >>> Execution cannot reach this statement: "return -22;".
> > 148 return -EINVAL;
>
> This is a false positive.
>
> abi_idx value could end up matching this condition "(abi_idx < 0 || abi_idx >= FFA_ERRMAP_COUNT)".
>
> This happens when ffa_id value is above the allowed bounds. Example: when ffa_id is 0x50 or 0x80
>
> ffa_print_error_log(0x50, ...); /* exceeding lower bound */
> ffa_print_error_log(0x80, ...); /* exceeding upper bound */
>
> In these cases "return -EINVAL;" is executed.

So those invalid values aren't caught by the previous check that ffa_id
falls within FFA_FIRST_ID to FFA_LAST_ID ?

> > ...
> > ________________________________________________________________________________________________________
> > *** CID 464360: Control flow issues (NO_EFFECT)
> > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 207 in ffa_get_version_hdlr()
> > 201 major = GET_FFA_MAJOR_VERSION(res.a0);
> > 202 minor = GET_FFA_MINOR_VERSION(res.a0);
> > 203
> > 204 log_debug("FF-A driver %d.%d\nFF-A framework %d.%d\n",
> > 205 FFA_MAJOR_VERSION, FFA_MINOR_VERSION, major, minor);
> > 206
> > >>> CID 464360: Control flow issues (NO_EFFECT)
> > >>> This greater-than-or-equal-to-zero comparison of an unsigned value is always true. "minor >= 0".
> > 207 if (major == FFA_MAJOR_VERSION && minor >= FFA_MINOR_VERSION) {
>
> Providing the facts that:
>
> #define FFA_MINOR_VERSION (0)
> u16 minor;
>
> Yes, currently this condition is always true: minor >= FFA_MINOR_VERSION
>
> However, we might upgrade FFA_MINOR_VERSION in the future. If we remove the "minor >= FFA_MINOR_VERSION" ,
> non compatible versions could pass which we don't want.
>
> To keep this code scalable, I think it's better to keep this condition.

OK, thanks this makes sense as an intentional change for future sanity
checking.

> > ________________________________________________________________________________________________________
> > *** CID 464359: (PASS_BY_VALUE)
> > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 168 in invoke_ffa_fn()
> > 162 * @args: FF-A ABI arguments to be copied to Xn registers
> > 163 * @res: FF-A ABI return data to be copied from Xn registers
> > 164 *
> > 165 * Calls low level SMC implementation.
> > 166 * This function should be implemented by the user driver.
> > 167 */
> > >>> CID 464359: (PASS_BY_VALUE)
> > >>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by value, which exceeds the low threshold of 128 bytes.
> > 168 void __weak invoke_ffa_fn(ffa_value_t args, ffa_value_t *res)
>
> We are using invoke_ffa_fn with the same arguments as in linux. The aim is to use the same interfaces as in the Linux FF-A
> driver to make porting code easier.
>
> In Linux, args is passed by value [1].
> ffa_value_t is a structure with 18 "unsigned long" fields. So, the size is fixed.
>
> [1]: invoke_ffa_fn arguments in the Linux FF-A driver
>
> https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/driver.c#L115
> https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/driver.c#L54
> https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/common.h#L15
>
> [2]: include/linux/arm-smccc.h

So this is intentional, OK.

>
> > 169 {
> > 170 }
> > 171
> > 172 /**
> > 173 * ffa_get_version_hdlr() - FFA_VERSION handler function
> > /drivers/firmware/arm-ffa/ffa-emul-uclass.c: 673 in invoke_ffa_fn()
> > 667 * invoke_ffa_fn() - SMC wrapper
> > 668 * @args: FF-A ABI arguments to be copied to Xn registers
> > 669 * @res: FF-A ABI return data to be copied from Xn registers
> > 670 *
> > 671 * Calls the emulated SMC call.
> > 672 */
> > >>> CID 464359: (PASS_BY_VALUE)
> > >>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by value, which exceeds the low threshold of 128 bytes.
> > 673 void invoke_ffa_fn(ffa_value_t args, ffa_value_t *res)
>
> Same feedback as above.

Thanks. I'll update the last 3 CIDs shortly.

--
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]