USB: gadgetfs: Fix use-after-free in gadgetfs_kill_sb

When gadgetfs_fill_super() fails, it's error path calls put_dev() which drops refcount inside the_device to 0 and frees the objet. But the_device pointer is not cleared, leading to point at freed memory.

VFS will then call gadgetfs_kill_sb() after mount failure leading to put_dev() to be called on the already freed pointer.

Fix by setting the_device = NULL during error path before calling put_dev() inside gadgetfs_fill_super() so that gadgetfs_kill_sb() skips put_dev().

However, if the fault injection from syzbot failed and it began the open()/write()/close()/unmount() sequence then close() and umount() will each trigger the refcount to drop once via put_dev(). This will still cause UAF since refcount is only incremented once on creation but decremented twice.

Reported-by [email protected]

Link https://syzkaller.appspot.com/bug?extid=4a5c87a01894ca37f25c

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Signed-off-by Rafael Alejandro Diaz Cruz <[email protected]>

---
 drivers/usb/gadget/legacy/inode.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/usb/gadget/legacy/inode.c +1 -0

index d87a8ab51510..77efa984ce84 100644
@@ -2059,6 +2059,7 @@ gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc)
 	rc = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations);
 	if (rc) {
 		put_dev(dev);
+		the_device = NULL;
 		goto Enomem;
 	}
 

drivers/usb/gadget/legacy/inode.c +0 -0

 	rc = 0;
 	goto Done;
 

Signature · 1 line

2.43.0

Quoted 13 lines · Rafael Alejandro Diaz Cruz

When gadgetfs_fill_super() fails, it's error path calls put_dev() which drops refcount inside the_device to 0 and frees the objet. But the_device pointer is not cleared, leading to point at freed memory.

VFS will then call gadgetfs_kill_sb() after mount failure leading to put_dev() to be called on the already freed pointer.

Fix by setting the_device = NULL during error path before calling put_dev() inside gadgetfs_fill_super() so that gadgetfs_kill_sb() skips put_dev().

Quoted 6 lines

However, if the fault injection from syzbot failed and it began the open()/write()/close()/unmount() sequence then close() and umount() will each trigger the refcount to drop once via put_dev(). This will still cause UAF since refcount is only incremented once on creation but decremented twice.

I don't understand this last paragraph at all. What fault injection from syzbot are you talking about? The earlier part of the description doesn't mention syzbot at all.

Why do you spell "unmount" the first time with an 'n' but "umount" the second time without an 'n'?

Is there any reason why close and unmount shouldn't both do a put_dev()? Doesn't the open increment the refcount to 2, so close and unmount will set it to 0, causing a deallocation but not a UAF?

Why is the refcount incremented upon creation? Normally refcounts are created with an initial value of 1 so they don't need to be incremented.

Quoted 21 lines

Reported-by [email protected]

Link https://syzkaller.appspot.com/bug?extid=4a5c87a01894ca37f25c

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Signed-off-by Rafael Alejandro Diaz Cruz <[email protected]>

---
 drivers/usb/gadget/legacy/inode.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/usb/gadget/legacy/inode.c +1 -0

index d87a8ab51510..77efa984ce84 100644
@@ -2059,6 +2059,7 @@ gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc)
      rc = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations);
      if (rc) {
              put_dev(dev);
+             the_device = NULL;
              goto Enomem;
      }
 

drivers/usb/gadget/legacy/inode.c gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc) · line 2070

Quoted 4 lines

drivers/usb/gadget/legacy/inode.c +1 -0

@@ -2066,6 +2067,7 @@ gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc)
       * from binding to a controller.
       */
      the_device = dev;
+     get_dev(dev);

Does this have something to do with that mysterious last paragraph in the description? I can't see any relation between the two. In particular, that paragraph doesn't say anything about adding a get_dev().

Alan Stern

Quoted 5 lines

drivers/usb/gadget/legacy/inode.c +0 -1

      rc = 0;
      goto Done;
 
--

2.43.0

Hello,
Sorry for the confusion. I assume I can just address the concerns here and create another patch once the message is clear.

I suppose there are actually two errors but because they are triggered by the same syzkaller reproducer, I'm treating them as one. The initial problem is the UAF on the_device pointer when put_dev() is called both by the error path of gadgetfs_fill_super() and gadgetfs_kill_sb(). This is triggered by the fault_injection in the reproducer. I'll assume that one is clear.

The second underlying issue is still related to the_device but not itself, rather the_device->count variable keeping the references. The syzkaller reproducer still triggers the same fault_injection in the reproducer but the fault_injection might fail which leads to the success path being taken. This means that the gadgetfs_fill_super() will succeed and the the_device->count will be initialized with 1 as you had mentioned. However, at the end of the reproducer close() and umount() will each be called. This leads to the following:

close() -> dev_releave() -> put_dev()
umount -> gadgetfs_kill_sb() -> put_dev()

the_device->count == 1 at the start but is decremented twice. refcount < 0 triggers second UAF.

This is fixed by incrementing the reference on the success path of gadgetfs_fill_super(). That is what the get_dev(dev); call is for.

As a side note, I'm part of the Linux Kernel Mentorship and I appreciate your feedback! Please don't hold back on the suggestions. Every bit helps me!

Quoted 5 lines · Rafael Alejandro Díaz Cruz

Hello,
Sorry for the confusion. I assume I can just address the concerns here and create another patch once the message is clear.

Quoted 7 lines

I suppose there are actually two errors but because they are triggered by the same syzkaller reproducer, I'm treating them as one. The initial problem is the UAF on the_device pointer when put_dev() is called both by the error path of gadgetfs_fill_super() and gadgetfs_kill_sb(). This is triggered by the fault_injection in the reproducer. I'll assume that one is clear.

Yes, that's okay.

Quoted 11 lines

The second underlying issue is still related to the_device but not itself, rather the_device->count variable keeping the references. The syzkaller reproducer still triggers the same fault_injection in the reproducer but the fault_injection might fail which leads to the success path being taken. This means that the gadgetfs_fill_super() will succeed and the the_device->count will be initialized with 1 as you had mentioned. However, at the end of the reproducer close() and umount() will each be called. This leads to the following:

Quoted 5 lines

close() -> dev_releave() -> put_dev()
umount -> gadgetfs_kill_sb() -> put_dev()

the_device->count == 1 at the start but is decremented twice. refcount < 0 triggers second UAF.

But gadget_dev_open() increments the refcount (it calls get_dev()). The increment caused by opening the device cancels out the decrement caused by closing it. And the initial assignment to 1 caused by creating it cancels out the final decrement caused by gadgetfs_kill_sb(). So there shouldn't be any erroneous accesses.

Quoted 3 lines

This is fixed by incrementing the reference on the success path of gadgetfs_fill_super(). That is what the get_dev(dev); call is for.

If you do that, what will cancel out the increment caused by gadget_dev_open()?

Quoted 3 lines

As a side note, I'm part of the Linux Kernel Mentorship and I appreciate your feedback! Please don't hold back on the suggestions. Every bit helps me!

Don't mention it; we all have to start somewhere!

Alan Stern

After spending a couple hours staring at the code and asking claude for things I might have missed, I did come up with a better explanation of how the error happens, but not why it happens.

I began by removing the get_dev(dev) line I previously added and keeping a mental score of all the times that refcount is incremented and decremented and found that in most cases, they are even. Except in one case inside gadgetfs_bind():

static int gadgetfs_bind(struct usb_gadget *gadget, struct

usb_gadget_driver *driver) {
     struct dev_data *dev = the_device;
// ........
     + get_dev(dev)  // HERE
     if (!dev->req);
         goto enomem;

     if (activate_ep_files (dev) < 0)
        goto enomem;

// ......
- get_dev (dev); // Move this up
return 0;

enomem:

     gadgetfs_unbind (gadget);
     return -ENOMEM;
}

Somehow the reproducer is triggering one of those two if conditions and sending the execution down the error path directly to the gadgetfs_unbind(gadget) and skipping the get_dev(dev) right before it.

Inside gadgetfs_unbind() we call put_dev(dev) which would be an uneven decrement. I can confirm this was the problem by first making the change above and then triggering the reproducer on my local QEMU which does not trigger the UAF.

Yet, I don't understand why this is being triggered since from what I can see, none of the syscalls coming from the reproducer are directly related to this. And these .._bind(), ..._unbind() functions are not part of the UAF stack trace but rather, something that happens in between the reproducer triggered by the usb protocol.

Not sure if this is good enough. I don't know what else to look into.

I kept going at it and finally realized what was going on.

I added some logging statements as you suggested and found that my 'how this bug is caused' is correct. It does come from calling get_dev() after the error paths, and the simple solution was to move get_dev() right after the set_gadget_data().

As for 'why' the bug is happening, it basically comes from the reproducer using failslab to force kmalloc() to fail inside of usb_ep_alloc_request(). This will cause the following error path to be taken:

dev->req = usb_ep_alloc_request (gadget->ep0, GFP_KERNEL);
if (!dev->req) // This is true on failslab

     goto enomem;

This will then skip get_dev() and go directly to gadgetfs_unbind() -> put_dev()
creating the unbalanced refcount.

It was tricky to understand given that it was failing only on the N-th failslab attempt. Luckily gdb could do all the things I needed.

I'm also happy to share that even Claude was not able to figure this out and could only confirm this was the cause after I pointed it out.

I'm certain of what I found and will proceed to create the next patch with an updated message.

Please let me know if there is anything else I should add. I'll wait for your response before sending it out.