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 ?
> kvfree(bin.base);
> + memset(ccsdata, 0, sizeof(*ccsdata));
>
> return rval;
> }
> --
> 2.39.5
>
--
Kind Regards
Mehdi Djait
