Re: [PATCH 6.11 066/184] media: dvbdev: prevent the risk of out of memory access

Re: [PATCH 6.11 066/184] media: dvbdev: prevent the risk of out of memory access

[Nathan Chancellor]

On Tue, Nov 12, 2024 at 11:20:24AM +0100, Greg Kroah-Hartman wrote:
> 6.11-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> [ Upstream commit 972e63e895abbe8aa1ccbdbb4e6362abda7cd457 ]
> 
> The dvbdev contains a static variable used to store dvb minors.
> 
> The behavior of it depends if CONFIG_DVB_DYNAMIC_MINORS is set
> or not. When not set, dvb_register_device() won't check for
> boundaries, as it will rely that a previous call to
> dvb_register_adapter() would already be enforcing it.
> 
> On a similar way, dvb_device_open() uses the assumption
> that the register functions already did the needed checks.
> 
> This can be fragile if some device ends using different
> calls. This also generate warnings on static check analysers
> like Coverity.
> 
> So, add explicit guards to prevent potential risk of OOM issues.
> 
> Fixes: 5dd3f3071070 ("V4L/DVB (9361): Dynamic DVB minor allocation")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/media/dvb-core/dvbdev.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index b43695bc51e75..14f323fbada71 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -86,10 +86,15 @@ static DECLARE_RWSEM(minor_rwsem);
>  static int dvb_device_open(struct inode *inode, struct file *file)
>  {
>  	struct dvb_device *dvbdev;
> +	unsigned int minor = iminor(inode);
> +
> +	if (minor >= MAX_DVB_MINORS)
> +		return -ENODEV;
>  
>  	mutex_lock(&dvbdev_mutex);
>  	down_read(&minor_rwsem);
> -	dvbdev = dvb_minors[iminor(inode)];
> +
> +	dvbdev = dvb_minors[minor];
>  
>  	if (dvbdev && dvbdev->fops) {
>  		int err = 0;
> @@ -525,7 +530,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
>  	for (minor = 0; minor < MAX_DVB_MINORS; minor++)
>  		if (!dvb_minors[minor])
>  			break;
> -	if (minor == MAX_DVB_MINORS) {
> +	if (minor >= MAX_DVB_MINORS) {
>  		if (new_node) {
>  			list_del(&new_node->list_head);
>  			kfree(dvbdevfops);
> @@ -540,6 +545,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
>  	}
>  #else
>  	minor = nums2minor(adap->num, type, id);
> +	if (minor >= MAX_DVB_MINORS) {
> +		dvb_media_device_free(dvbdev);
> +		list_del(&dvbdev->list_head);
> +		kfree(dvbdev);
> +		*pdvbdev = NULL;
> +		mutex_unlock(&dvbdev_register_lock);
> +		return ret;

This needs commit a4aebaf6e6ef ("media: dvbdev: fix the logic when
DVB_DYNAMIC_MINORS is not set"), otherwise there is a warning with
certain configurations when building with clang:

  drivers/media/dvb-core/dvbdev.c:554:10: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
    554 |                 return ret;
        |                        ^~~
  drivers/media/dvb-core/dvbdev.c:463:13: note: initialize the variable 'ret' to silence this warning
    463 |         int id, ret;
        |                    ^
        |                     = 0
  1 warning generated.

I was somewhat surprised when this warning showed up in my stable
builds, until I realized that change does not have a Fixes tag like it
really should have...

Cheers,
Nathan

> +	}
>  #endif

>  	dvbdev->minor = minor;
>  	dvb_minors[minor] = dvb_device_get(dvbdev);
> -- 
> 2.43.0
> 
> 
> 

Re: [PATCH 6.11 066/184] media: dvbdev: prevent the risk of out of memory access

[Greg Kroah-Hartman]

On Sun, Nov 17, 2024 at 05:33:38PM -0700, Nathan Chancellor wrote:
> On Tue, Nov 12, 2024 at 11:20:24AM +0100, Greg Kroah-Hartman wrote:
> > 6.11-stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > 
> > [ Upstream commit 972e63e895abbe8aa1ccbdbb4e6362abda7cd457 ]
> > 
> > The dvbdev contains a static variable used to store dvb minors.
> > 
> > The behavior of it depends if CONFIG_DVB_DYNAMIC_MINORS is set
> > or not. When not set, dvb_register_device() won't check for
> > boundaries, as it will rely that a previous call to
> > dvb_register_adapter() would already be enforcing it.
> > 
> > On a similar way, dvb_device_open() uses the assumption
> > that the register functions already did the needed checks.
> > 
> > This can be fragile if some device ends using different
> > calls. This also generate warnings on static check analysers
> > like Coverity.
> > 
> > So, add explicit guards to prevent potential risk of OOM issues.
> > 
> > Fixes: 5dd3f3071070 ("V4L/DVB (9361): Dynamic DVB minor allocation")
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Signed-off-by: Sasha Levin <sashal@kernel.org>
> > ---
> >  drivers/media/dvb-core/dvbdev.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> > index b43695bc51e75..14f323fbada71 100644
> > --- a/drivers/media/dvb-core/dvbdev.c
> > +++ b/drivers/media/dvb-core/dvbdev.c
> > @@ -86,10 +86,15 @@ static DECLARE_RWSEM(minor_rwsem);
> >  static int dvb_device_open(struct inode *inode, struct file *file)
> >  {
> >  	struct dvb_device *dvbdev;
> > +	unsigned int minor = iminor(inode);
> > +
> > +	if (minor >= MAX_DVB_MINORS)
> > +		return -ENODEV;
> >  
> >  	mutex_lock(&dvbdev_mutex);
> >  	down_read(&minor_rwsem);
> > -	dvbdev = dvb_minors[iminor(inode)];
> > +
> > +	dvbdev = dvb_minors[minor];
> >  
> >  	if (dvbdev && dvbdev->fops) {
> >  		int err = 0;
> > @@ -525,7 +530,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
> >  	for (minor = 0; minor < MAX_DVB_MINORS; minor++)
> >  		if (!dvb_minors[minor])
> >  			break;
> > -	if (minor == MAX_DVB_MINORS) {
> > +	if (minor >= MAX_DVB_MINORS) {
> >  		if (new_node) {
> >  			list_del(&new_node->list_head);
> >  			kfree(dvbdevfops);
> > @@ -540,6 +545,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
> >  	}
> >  #else
> >  	minor = nums2minor(adap->num, type, id);
> > +	if (minor >= MAX_DVB_MINORS) {
> > +		dvb_media_device_free(dvbdev);
> > +		list_del(&dvbdev->list_head);
> > +		kfree(dvbdev);
> > +		*pdvbdev = NULL;
> > +		mutex_unlock(&dvbdev_register_lock);
> > +		return ret;
> 
> This needs commit a4aebaf6e6ef ("media: dvbdev: fix the logic when
> DVB_DYNAMIC_MINORS is not set"), otherwise there is a warning with
> certain configurations when building with clang:
> 
>   drivers/media/dvb-core/dvbdev.c:554:10: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
>     554 |                 return ret;
>         |                        ^~~
>   drivers/media/dvb-core/dvbdev.c:463:13: note: initialize the variable 'ret' to silence this warning
>     463 |         int id, ret;
>         |                    ^
>         |                     = 0
>   1 warning generated.
> 
> I was somewhat surprised when this warning showed up in my stable
> builds, until I realized that change does not have a Fixes tag like it
> really should have...

Now queued up, thanks.

greg k-h