linux-kernel - Re: [PATCH] media input infrastructure:tw686x: Fix of possibleinconsistent memory deallocation and/or race condition by implementation of custom video_device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <40d14e23-636e-ed8a-6608-99427f5b8169@xs4all.nl>
Date:   Tue, 30 Jul 2019 10:15:36 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Mark Balantzyan <mbalant3@...il.com>, ezequiel@...guardiasur.com.ar
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media input infrastructure:tw686x: Fix of
 possibleinconsistent memory deallocation and/or race condition by
 implementation of custom video_device_release function in tw686x driver

Hi Mark,

Please, please read this first before posting patches:

https://kernelnewbies.org/FirstKernelPatch

And don't use insanely long subject lines in your email.

This patch is nonsense. As said before, you need to override the release() callback
of struct video_device with a tw686x-specific function that frees the dma memory and
calls video_device_release() at the end to free the struct video_device itself.

This release() callback is called by the V4L2 framework when the last user of the
device closes its filehandle, so that's a good point to free all the memory. Doing
it earlier (as the current code does) runs the risk that someone might still access
that memory, and you don't want that.

Regards,

	Hans

On 7/29/19 10:09 PM, Mark Balantzyan wrote:
> 
> Possible inconsistent memory deallocation and/or race conditions were detected specifically with respect to remaining open handles to the video device handled by the tw686x driver. This patch
> addresses this by implementing a revised independent instance of the video_device_release function to free the remaining resources and memory where the last open handle(s) is/were closed.
> 
> Signed-off-by: Mark Balantzyan <mbalant3@...il.com>
> 
> ---
> 
>  drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 3a06c000..29e10c85 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -1151,18 +1151,25 @@ void tw686x_video_irq(struct tw686x_dev *dev, unsigned long requests,
>      }
>  }
> 
> +void video_device_release(tw686x_dev *dev) {
> +    for (int pb = 0; pb < 2; pb++) {
> +        dev->dma_ops->free(dev->video_channels,pb);
> +    }
> +    kfree(dev);
> +}
> +
>  void tw686x_video_free(struct tw686x_dev *dev)
>  {
> -    unsigned int ch, pb;
> +    unsigned int ch;
> 
>      for (ch = 0; ch < max_channels(dev); ch++) {
>          struct tw686x_video_channel *vc = &dev->video_channels[ch];
> 
>          video_unregister_device(vc->device);
> 
> -        if (dev->dma_ops->free)
> -            for (pb = 0; pb < 2; pb++)
> -                dev->dma_ops->free(vc, pb);
> +        if (dev->dma_ops->free) {
> +            video_device_release(dev);
> +        }
>      }
>  }
>