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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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