[2/3] atomisp: fix vfree of bogus data on unload

  We load the firmware once, set pointers to it and then at some point release
it. We should not be doing a vfree() on the pointers into the firmware.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---
 .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c |    2 --
 1 file changed, 2 deletions(-)
  

Hi Alan,

On Mon, Nov 06, 2017 at 11:36:45PM +0000, Alan wrote:
> We load the firmware once, set pointers to it and then at some point release
> it. We should not be doing a vfree() on the pointers into the firmware.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
>  .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> index 8158ea40d069..f181bd8fcee2 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> @@ -288,8 +288,6 @@ void sh_css_unload_firmware(void)
>  		for (i = 0; i < sh_css_num_binaries; i++) {
>  			if (fw_minibuffer[i].name)
>  				kfree((void *)fw_minibuffer[i].name);
> -			if (fw_minibuffer[i].buffer)
> -				vfree((void *)fw_minibuffer[i].buffer);

You shouldn't end up here if the firmware is just loaded once. If multiple
times, then yes.

The memory appears to have been allocated using kmalloc() in some cases.
How about kvfree(), or changing that kmalloc() to vmalloc()?

>  		}
>  		kfree(fw_minibuffer);
>  		fw_minibuffer = NULL;
>
  

On Tue, 14 Nov 2017 00:05:48 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> wrote:

> Hi Alan,
> 
> On Mon, Nov 06, 2017 at 11:36:45PM +0000, Alan wrote:
> > We load the firmware once, set pointers to it and then at some point release
> > it. We should not be doing a vfree() on the pointers into the firmware.
> > 
> > Signed-off-by: Alan Cox <alan@linux.intel.com>
> > ---
> >  .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c |    2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > index 8158ea40d069..f181bd8fcee2 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > @@ -288,8 +288,6 @@ void sh_css_unload_firmware(void)
> >  		for (i = 0; i < sh_css_num_binaries; i++) {
> >  			if (fw_minibuffer[i].name)
> >  				kfree((void *)fw_minibuffer[i].name);
> > -			if (fw_minibuffer[i].buffer)
> > -				vfree((void *)fw_minibuffer[i].buffer);  
> 
> You shouldn't end up here if the firmware is just loaded once. If multiple
> times, then yes.

You end up there when unloading the module.

> The memory appears to have been allocated using kmalloc() in some cases.
> How about kvfree(), or changing that kmalloc() to vmalloc()

I'll take a deeper look at what is going on.

Alan
  

On Tue, Nov 14, 2017 at 12:16:01AM +0000, Alan Cox wrote:
> On Tue, 14 Nov 2017 00:05:48 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> 
> > Hi Alan,
> > 
> > On Mon, Nov 06, 2017 at 11:36:45PM +0000, Alan wrote:
> > > We load the firmware once, set pointers to it and then at some point release
> > > it. We should not be doing a vfree() on the pointers into the firmware.
> > > 
> > > Signed-off-by: Alan Cox <alan@linux.intel.com>
> > > ---
> > >  .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c |    2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > > index 8158ea40d069..f181bd8fcee2 100644
> > > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
> > > @@ -288,8 +288,6 @@ void sh_css_unload_firmware(void)
> > >  		for (i = 0; i < sh_css_num_binaries; i++) {
> > >  			if (fw_minibuffer[i].name)
> > >  				kfree((void *)fw_minibuffer[i].name);
> > > -			if (fw_minibuffer[i].buffer)
> > > -				vfree((void *)fw_minibuffer[i].buffer);  
> > 
> > You shouldn't end up here if the firmware is just loaded once. If multiple
> > times, then yes.
> 
> You end up there when unloading the module.

Ah, that's for sure indeed. I thought loading would be already a challenge.
:-)

> 
> > The memory appears to have been allocated using kmalloc() in some cases.
> > How about kvfree(), or changing that kmalloc() to vmalloc()
> 
> I'll take a deeper look at what is going on.

Look for minibuffer in sh_css_load_blob_info(). The buffer field of the
struct is allocated using kmalloc, I wonder if changing that to vmalloc
would just address this.

The buffer is elsewhere allocated using vmalloc. I suspect that some of the
cleanup patches changed how this works but missed changing the other one.