| Message ID | 20241205094446.1491176-1-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/1] media: ccs: Clean up parsed CCS static data on parse failure | expand |
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@ixit.cz> > Fixes: a6b396f410b1 ("media: ccs: Add CCS static data parser library") > Cc: stable@vger.kernel.org > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > 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
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@ixit.cz> > > Fixes: a6b396f410b1 ("media: ccs: Add CCS static data parser library") > > Cc: stable@vger.kernel.org > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > 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? :-) > > > kvfree(bin.base); > > + memset(ccsdata, 0, sizeof(*ccsdata)); > > > > return rval; > > }
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@ixit.cz> > > > Fixes: a6b396f410b1 ("media: ccs: Add CCS static data parser library") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > 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@linux.intel.com> > > > > > > kvfree(bin.base); > > > + memset(ccsdata, 0, sizeof(*ccsdata)); > > > > > > return rval; -- Kind Regards Mehdi Djait
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: kvfree(bin.base); + memset(ccsdata, 0, sizeof(*ccsdata)); return rval; }
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@ixit.cz> Fixes: a6b396f410b1 ("media: ccs: Add CCS static data parser library") Cc: stable@vger.kernel.org Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- since v2: - Properly clean up after all error cases. drivers/media/i2c/ccs/ccs-data.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)