Hi Hans,
On Thu, Aug 03, 2023 at 09:31:13AM +0200, Hans Verkuil wrote:
> 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.
"rules" is actually NULL on the first round of parsing, which is used to
determine the size of the memory allocation for the resulting data
structure.
I guess it warns then about just referencing it while it isn't actually
accessed?
The bug should be fixed by this patch though. I'll see if the warning could
also be suppressed without making the code too awkward.
>
> 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;
> > }
>
--
Kind regards,
Sakari Ailus
