[PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure

[PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure

[Sakari Ailus]

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

Re: [PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure

[Mehdi Djait]

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

Re: [PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure

[Sakari Ailus]

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

Re: [PATCH v3 1/1] media: ccs: Clean up parsed CCS static data on parse failure

[Mehdi Djait]

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