* [PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure
@ 2024-12-05 9:44 Sakari Ailus
2024-12-05 9:54 ` Mehdi Djait
0 siblings, 1 reply; 4+ messages in thread
From: Sakari Ailus @ 2024-12-05 9:44 UTC (permalink / raw)
To: linux-media; +Cc: david, mehdi.djait
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:
kvfree(bin.base);
+ memset(ccsdata, 0, sizeof(*ccsdata));
return rval;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure 2024-12-05 9:44 [PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure Sakari Ailus @ 2024-12-05 9:54 ` Mehdi Djait 2024-12-05 11:57 ` Sakari Ailus 0 siblings, 1 reply; 4+ messages in thread From: Mehdi Djait @ 2024-12-05 9:54 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media, david 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure 2024-12-05 9:54 ` Mehdi Djait @ 2024-12-05 11:57 ` Sakari Ailus 2024-12-05 12:38 ` Mehdi Djait 0 siblings, 1 reply; 4+ messages in thread From: Sakari Ailus @ 2024-12-05 11:57 UTC (permalink / raw) To: Mehdi Djait; +Cc: linux-media, david 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; > > } -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure 2024-12-05 11:57 ` Sakari Ailus @ 2024-12-05 12:38 ` Mehdi Djait 0 siblings, 0 replies; 4+ messages in thread From: Mehdi Djait @ 2024-12-05 12:38 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media, david 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-12-05 12:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-05 9:44 [PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure Sakari Ailus 2024-12-05 9:54 ` Mehdi Djait 2024-12-05 11:57 ` Sakari Ailus 2024-12-05 12:38 ` Mehdi Djait
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.