Based on your explanation, file->private_data can not be NULL before call vbg_core_close_session method all the time, since file->private_data is always set. Am I right?
On Tue, May 28, 2019 at 9:19 PM Hans de Goede <hdego...@redhat.com> wrote: > > Hi, > > On 28-05-19 14:47, Young Xiao wrote: > > vbg_misc_device_close doesn't check that filp->private_data is non-NULL > > before trying to close_session, where vbg_core_close_session uses pointer > > session whithout checking, too. That can cause an oops in certain error > > conditions that can occur on open or lookup before the private_data is set. > > > > This vulnerability is similar to CVE-2011-1771. > > How is this in anyway related to CVE-2011-1771 ???? > > That CVE is about a filesystems lookup method including a direct open > of the file for performance reasons and if that direct open fails, doing > a fput, which is how it ends up with a file with private_date being NULL, > see: > > https://bugzilla.redhat.com/show_bug.cgi?id=703016 > > "the problem is that CIFS doesn't do O_DIRECT at all, so when you try to open > a file with it you get back -EINVAL. CIFS can also do open on lookup in some > cases. In that case, fput will be called on the filp, which has not yet had > its private_data set." > > The vboxguest code on the other hand is not a filessystem and as such > does not have a dentry lookup method at all! > > It's close method is part of the file_operations struct for a character > device using the misc_device framework which guarantees that private_data > is always set. > > Regards, > > Hans > > > > > > > > > Signed-off-by: Young Xiao <92siuy...@gmail.com> > > --- > > drivers/virt/vboxguest/vboxguest_linux.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/virt/vboxguest/vboxguest_linux.c > > b/drivers/virt/vboxguest/vboxguest_linux.c > > index 6e8c0f1..b03c16f 100644 > > --- a/drivers/virt/vboxguest/vboxguest_linux.c > > +++ b/drivers/virt/vboxguest/vboxguest_linux.c > > @@ -88,8 +88,10 @@ static int vbg_misc_device_user_open(struct inode > > *inode, struct file *filp) > > */ > > static int vbg_misc_device_close(struct inode *inode, struct file *filp) > > { > > - vbg_core_close_session(filp->private_data); > > - filp->private_data = NULL; > > + if (file->private_data != NULL) { > > + vbg_core_close_session(filp->private_data); > > + filp->private_data = NULL; > > + } > > return 0; > > } > > > > -- Best regards! Young -----------------------------------------------------------
