Hi Sakari,
On 02/08/2023 12:04, Sakari Ailus wrote:
> Fix the following smatch warning:
>
> drivers/media/i2c/ccs/ccs-data.c:524 ccs_data_parse_rules() warn: address
> of NULL pointer 'rules'
>
> The CCS static data rule parser does not check an if rule has been
> obtained before checking for other rule types (which depend on the if
> rule). In practice this means parsing invalid CCS static data could lead
> to dereferencing a NULL pointer.
>
> Reported-by: Hans Verkuil <hverkuil@xxxxxxxxx>
> Fixes: a6b396f410b1 ("media: ccs: Add CCS static data parser library")
> Cc: stable@xxxxxxxxxxxxxxx # for 5.11 and up
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> drivers/media/i2c/ccs/ccs-data.c | 94 +++++++++++++++++---------------
> 1 file changed, 49 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/media/i2c/ccs/ccs-data.c b/drivers/media/i2c/ccs/ccs-data.c
> index 45f2b2f55ec5..5e3ca02112f1 100644
> --- a/drivers/media/i2c/ccs/ccs-data.c
> +++ b/drivers/media/i2c/ccs/ccs-data.c
> @@ -464,8 +464,7 @@ static int ccs_data_parse_rules(struct bin_container *bin,
> rule_payload = __rule_type + 1;
> rule_plen2 = rule_plen - sizeof(*__rule_type);
>
> - switch (*__rule_type) {
> - case CCS_DATA_BLOCK_RULE_ID_IF: {
> + if (*__rule_type == CCS_DATA_BLOCK_RULE_ID_IF) {
> const struct __ccs_data_block_rule_if *__if_rules =
> rule_payload;
> const size_t __num_if_rules =
> @@ -514,49 +513,54 @@ static int ccs_data_parse_rules(struct bin_container *bin,
> rules->if_rules = if_rule;
> rules->num_if_rules = __num_if_rules;
> }
> - break;
> - }
> - case CCS_DATA_BLOCK_RULE_ID_READ_ONLY_REGS:
> - rval = ccs_data_parse_reg_rules(bin, &rules->read_only_regs,
> - &rules->num_read_only_regs,
> - rule_payload,
> - rule_payload + rule_plen2,
> - dev);
> - if (rval)
> - return rval;
> - break;
> - case CCS_DATA_BLOCK_RULE_ID_FFD:
> - rval = ccs_data_parse_ffd(bin, &rules->frame_format,
> - rule_payload,
> - rule_payload + rule_plen2,
> - dev);
> - if (rval)
> - return rval;
> - break;
> - case CCS_DATA_BLOCK_RULE_ID_MSR:
> - rval = ccs_data_parse_reg_rules(bin,
> - &rules->manufacturer_regs,
> - &rules->num_manufacturer_regs,
> - rule_payload,
> - rule_payload + rule_plen2,
> - dev);
> - if (rval)
> - return rval;
> - break;
> - case CCS_DATA_BLOCK_RULE_ID_PDAF_READOUT:
> - rval = ccs_data_parse_pdaf_readout(bin,
> - &rules->pdaf_readout,
> - rule_payload,
> - rule_payload + rule_plen2,
> - dev);
> - if (rval)
> - return rval;
> - break;
> - default:
> - dev_dbg(dev,
> - "Don't know how to handle rule type %u!\n",
> - *__rule_type);
> - return -EINVAL;
> + } else {
> + /* Check there was an if rule before any other rules */
> + if (bin->base && !rules)
> + return -EINVAL;
> +
> + switch (*__rule_type) {
> + case CCS_DATA_BLOCK_RULE_ID_READ_ONLY_REGS:
> + rval = ccs_data_parse_reg_rules(bin, &rules->read_only_regs,
> + &rules->num_read_only_regs,
> + rule_payload,
> + rule_payload + rule_plen2,
> + dev);
I still get the same smatch warning:
drivers/media/i2c/ccs/ccs-data.c:524 ccs_data_parse_rules() warn: address of NULL pointer 'rules'
Shouldn't the 'if' above simply read: 'if (!rules)'?
With that change the smatch warning disappears.
Regards,
Hans
> + if (rval)
> + return rval;
> + break;
> + case CCS_DATA_BLOCK_RULE_ID_FFD:
> + rval = ccs_data_parse_ffd(bin, &rules->frame_format,
> + rule_payload,
> + rule_payload + rule_plen2,
> + dev);
> + if (rval)
> + return rval;
> + break;
> + case CCS_DATA_BLOCK_RULE_ID_MSR:
> + rval = ccs_data_parse_reg_rules(bin,
> + &rules->manufacturer_regs,
> + &rules->num_manufacturer_regs,
> + rule_payload,
> + rule_payload + rule_plen2,
> + dev);
> + if (rval)
> + return rval;
> + break;
> + case CCS_DATA_BLOCK_RULE_ID_PDAF_READOUT:
> + rval = ccs_data_parse_pdaf_readout(bin,
> + &rules->pdaf_readout,
> + rule_payload,
> + rule_payload + rule_plen2,
> + dev);
> + if (rval)
> + return rval;
> + break;
> + default:
> + dev_dbg(dev,
> + "Don't know how to handle rule type %u!\n",
> + *__rule_type);
> + return -EINVAL;
> + }
> }
> __next_rule = __next_rule + rule_hlen + rule_plen;
> }
