fix a file reference leak in drivers/misc/ntsync.c

Commit Message

Al Viro Jan. 15, 2025, 2:50 a.m. UTC

struct ntsync_obj contains a reference to struct file
and that reference contributes to refcount - ntsync_alloc_obj()
grabs it.  Normally the object is destroyed (and reference
to obj->file dropped) in ntsync_obj_release().  However, in
case of ntsync_obj_get_fd() failure the object is destroyed
directly by its creator.

	That case should also drop obj->file; plain kfree(obj)
is not enough there - it ends up leaking struct file * reference.

	Take that logics into a helper (ntsync_free_obj()) and
use it in both codepaths that destroy ntsync_obj instances.

Fixes: b46271ec40a05 "ntsync: Introduce NTSYNC_IOC_CREATE_SEM"
Signed-off-by: Al Viro <[e-mail]>
---

Comments

Elizabeth Figura Jan. 15, 2025, 6:20 p.m. UTC | #1

On Tuesday, 14 January 2025 20:50:02 CST Al Viro wrote:
> 	struct ntsync_obj contains a reference to struct file
> and that reference contributes to refcount - ntsync_alloc_obj()
> grabs it.  Normally the object is destroyed (and reference
> to obj->file dropped) in ntsync_obj_release().  However, in
> case of ntsync_obj_get_fd() failure the object is destroyed
> directly by its creator.
> 
> 	That case should also drop obj->file; plain kfree(obj)
> is not enough there - it ends up leaking struct file * reference.
> 
> 	Take that logics into a helper (ntsync_free_obj()) and
> use it in both codepaths that destroy ntsync_obj instances.
> 
> Fixes: b46271ec40a05 "ntsync: Introduce NTSYNC_IOC_CREATE_SEM"
> Signed-off-by: Al Viro <[e-mail]>
> ---

Reviewed-by: Elizabeth Figura <[e-mail]>


---

Thanks for catching this. There's a similar problem with the other newly introduced object types in the char-misc-next tree (and this patch doesn't apply cleanly there anyway). I'll send a similar patch for those, unless you have one already.

Greg Kroah-Hartman Jan. 15, 2025, 6:43 p.m. UTC | #2

On Wed, Jan 15, 2025 at 12:20:34PM -0600, Elizabeth Figura wrote:
> On Tuesday, 14 January 2025 20:50:02 CST Al Viro wrote:
> > 	struct ntsync_obj contains a reference to struct file
> > and that reference contributes to refcount - ntsync_alloc_obj()
> > grabs it.  Normally the object is destroyed (and reference
> > to obj->file dropped) in ntsync_obj_release().  However, in
> > case of ntsync_obj_get_fd() failure the object is destroyed
> > directly by its creator.
> > 
> > 	That case should also drop obj->file; plain kfree(obj)
> > is not enough there - it ends up leaking struct file * reference.
> > 
> > 	Take that logics into a helper (ntsync_free_obj()) and
> > use it in both codepaths that destroy ntsync_obj instances.
> > 
> > Fixes: b46271ec40a05 "ntsync: Introduce NTSYNC_IOC_CREATE_SEM"
> > Signed-off-by: Al Viro <[e-mail]>
> > ---
> 
> Reviewed-by: Elizabeth Figura <[e-mail]>
> 
> 
> ---
> 
> Thanks for catching this. There's a similar problem with the other newly introduced object types in the char-misc-next tree (and this patch doesn't apply cleanly there anyway). I'll send a similar patch for those, unless you have one already.

I already applied this and fixed up the fuzz there, it's now running
through 0-day testing...

thanks,

greg k-h

Patch

diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 4954553b7baa..6eb00d625bd1 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -97,13 +97,15 @@  static int ntsync_sem_post(struct ntsync_obj *sem, void __user *argp)
 	return ret;
 }
 
-static int ntsync_obj_release(struct inode *inode, struct file *file)
+static void ntsync_free_obj(struct ntsync_obj *obj)
 {
-	struct ntsync_obj *obj = file->private_data;
-
 	fput(obj->dev->file);
 	kfree(obj);
+}
 
+static int ntsync_obj_release(struct inode *inode, struct file *file)
+{
+	ntsync_free_obj(file->private_data);
 	return 0;
 }
 
@@ -183,7 +185,7 @@  static int ntsync_create_sem(struct ntsync_device *dev, void __user *argp)
 	sem->u.sem.max = args.max;
 	fd = ntsync_obj_get_fd(sem);
 	if (fd < 0) {
-		kfree(sem);
+		ntsync_free_obj(sem);
 		return fd;
 	}