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

[Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>]

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 ?

I had a closer look at that and I agree that the deadcode defect is legitimate.
I already provided a fix [1].

[1]: https://lore.kernel.org/all/20231020131533.239591-1-abdellatif.elkhlifi@arm.com/

> 
> > > ... 
> > > ________________________________________________________________________________________________________
> > > *** 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.

Thanks Tom :)

Cheers,
Abdellatif