Hi Sakari,
On Thu, Dec 05, 2024 at 11:57:00AM +0000, Sakari Ailus wrote:
> Hi Mehdi,
>
> Thanks for the review.
>
> On Thu, Dec 05, 2024 at 10:54:10AM +0100, Mehdi Djait wrote:
> > Hi Sakari,
> >
> > On Thu, Dec 05, 2024 at 11:44:46AM +0200, Sakari Ailus wrote:
> > > ccs_data_parse() releases the allocated in-memory data structure when the
> > > parser fails, but it does not clean up parsed metadata that is there to
> > > help access the actual data. Do that, in order to return the data
> > > structure in a sane state.
> > >
> > > Reported-by: David Heidelberg <david@xxxxxxx>
> > > Fixes: a6b396f410b1 ("media: ccs: Add CCS static data parser library")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > ---
> > > since v2:
> > >
> > > - Properly clean up after all error cases.
> > >
> > > drivers/media/i2c/ccs/ccs-data.c | 12 +++++++-----
> > > 1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ccs/ccs-data.c b/drivers/media/i2c/ccs/ccs-data.c
> > > index 9d42137f4799..2591dba51e17 100644
> > > --- a/drivers/media/i2c/ccs/ccs-data.c
> > > +++ b/drivers/media/i2c/ccs/ccs-data.c
> > > @@ -10,6 +10,7 @@
> > > #include <linux/limits.h>
> > > #include <linux/mm.h>
> > > #include <linux/slab.h>
> > > +#include <linux/string.h>
> > >
> > > #include "ccs-data-defs.h"
> > >
> > > @@ -948,15 +949,15 @@ int ccs_data_parse(struct ccs_data_container *ccsdata, const void *data,
> > >
> > > rval = __ccs_data_parse(&bin, ccsdata, data, len, dev, verbose);
> > > if (rval)
> > > - return rval;
> > > + goto out_cleanup;
> > >
> > > rval = bin_backing_alloc(&bin);
> > > if (rval)
> > > - return rval;
> > > + goto out_cleanup;
> > >
> > > rval = __ccs_data_parse(&bin, ccsdata, data, len, dev, false);
> > > if (rval)
> > > - goto out_free;
> > > + goto out_cleanup;
> > >
> > > if (verbose && ccsdata->version)
> > > print_ccs_data_version(dev, ccsdata->version);
> > > @@ -965,15 +966,16 @@ int ccs_data_parse(struct ccs_data_container *ccsdata, const void *data,
> > > rval = -EPROTO;
> > > dev_dbg(dev, "parsing mismatch; base %p; now %p; end %p\n",
> > > bin.base, bin.now, bin.end);
> > > - goto out_free;
> > > + goto out_cleanup;
> > > }
> > >
> > > ccsdata->backing = bin.base;
> > >
> > > return 0;
> > >
> > > -out_free:
> > > +out_cleanup:
> >
> > Don't you think some kind of logging or at least a dev_dbg() would be
> > helpful here to let the user know that ccs_data_parse() failed ?
>
> Good question. I think it's a good idea to print a warning, probably on
> warning level. I alsoI think it should be a separate patch.
>
> Could you write one? :-)
Yes, I will take care of it.
With that
Reviewed-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxxxxxx>
>
> >
> > > kvfree(bin.base);
> > > + memset(ccsdata, 0, sizeof(*ccsdata));
> > >
> > > return rval;
--
Kind Regards
Mehdi Djait
