Messages in this thread | | | Subject | Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock | From | Mukesh Ojha <> | Date | Wed, 24 Apr 2019 19:39:03 +0530 |
| |
On 4/24/2019 6:37 PM, Al Viro wrote: > On Wed, Apr 24, 2019 at 05:40:40PM +0530, Mukesh Ojha wrote: >> Al, >> >> i tried to put traceprintk inside ioctl after fdget and fdput on a simple >> call of open => ioctl => close > in a loop, and multithreaded, presumably? > >> on /dev/uinput. >> >> uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <= f_count >>> <After fdget() >> uinput-532 [002] .... 45.312055: SYSC_ioctl: 2 >> <After fdput() >> uinput-532 [004] .... 45.313766: uinput_open: uinput: 1 >> uinput-532 [004] .... 45.313783: SYSC_ioctl: 1 >> uinput-532 [004] .... 45.313788: uinput_ioctl_handler: >> uinput: uinput_ioctl_handler, 1 >> uinput-532 [004] .... 45.313835: SYSC_ioctl: 1 >> uinput-532 [004] .... 45.313843: uinput_release: uinput: 0 >> >> >> So while a ioctl is running the f_count is 1, so a fput could be run and do >> atomic_long_dec_and_test >> this could call release right ? > Look at ksys_ioctl(): > int ksys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg) > { > int error; > struct fd f = fdget(fd); > an error or refcount bumped > if (!f.file) > return -EBADF; > not an error, then. We know that ->release() won't be called > until we drop the reference we've just acquired. > error = security_file_ioctl(f.file, cmd, arg); > if (!error) > error = do_vfs_ioctl(f.file, fd, cmd, arg); > ... and we are done with calling ->ioctl(), so > fdput(f); > ... we drop the reference we'd acquired. > > Seeing refcount 1 inside ->ioctl() is possible, all right: > > CPU1: ioctl(2) resolves fd to struct file *, refcount 2 > CPU2: close(2) rips struct file * from descriptor table and does fput() to drop it; > refcount reaches 1 and fput() is done; no call of ->release() yet. > CPU1: we get arouund to ->ioctl(), where your trace sees refcount 1 > CPU1: done with ->ioctl(), drop our reference. *NOW* refcount gets to 0, and > ->release() is called.
Thanks for the detail reply, Al
This was my simple program no multithreading just to understand f_counting
int main() { int fd = open("/dev/uinput", O_WRONLY | O_NONBLOCK); ioctl(fd, UI_SET_EVBIT, EV_KEY); close(fd); return 0; }
uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <= f_count > <After fdget() uinput-532 [002] .... 45.312055: SYSC_ioctl: 2 <After fdput() uinput-532 [004] .... 45.313766: uinput_open: uinput: 1 /* This is from the uinput driver uinput_open()*/
=>>>> /* All the above calls happened for the open() in userspace*/
uinput-532 [004] .... 45.313783: SYSC_ioctl: 1 /* This print is for the trace, i put after fdget */ uinput-532 [004] .... 45.313788: uinput_ioctl_handler: uinput: uinput_ioctl_handler, 1 /* This print is from the uinput_ioctl driver */
uinput-532 [004] .... 45.313835: SYSC_ioctl: 1 /* This print is for the trace, i put after fdput*/ uinput-532 [004] .... 45.313843: uinput_release: uinput: 0 /* And this is from the close() */
Should fdget not suppose to increment the f_count here, as it is coming 1 ? This f_count to one is done at the open, but i have no idea how this below f_count 2 came before open() for this simple program.
uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <= f_count > <After fdget() uinput-532 [002] .... 45.312055: SYSC_ioctl: 2 <After fdput()
-Mukesh
> IOW, in your trace fput() has already been run by close(2); having somebody else > do that again while we are in ->ioctl() would be a bug (to start with, where > did they get that struct file * and why wasn't that reference contributing to > struct file refcount?) > > In all cases we only call ->release() once all references gone - both > the one(s) in descriptor tables and any transient ones acquired by > fdget(), etc. > > I would really like to see a reproducer for the original use-after-free report...
| |