Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Sorry to come late on this

On 4/25/2019 4:26 AM, Al Viro wrote:
On Wed, Apr 24, 2019 at 07:39:03PM +0530, Mukesh Ojha wrote:

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
Er...  So how does it manage to hit ioctl(2) before open(2)?  Confused...

I was confused too about this earlier, but after printing fd got to know this is not for the same fd opening for /dev/uinput, may it is for something while running the executable.


     <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.
If descriptor table is not shared, fdget() will simply return you the reference
from there, without bothering to bump the refcount.  _And_ having it marked
"don't drop refcount" in struct fd.

Rationale: since it's not shared, nobody other than our process can modify
it.  So unless we remove (and drop) the reference from it ourselves (which
we certainly have no business doing in ->ioctl() and do not do anywhere
in drivers/input), it will remain there until we return from syscall.

Nobody is allowed to modify descriptor table other than their own.
And if it's not shared, no other owners can appear while the only
existing one is in the middle of syscall other than clone() (with
CLONE_FILES in flags, at that).

For shared descriptor table fdget() bumps file refcount, since there
the reference in descriptor table itself could be removed and dropped
by another thread.

And fdget() marks whether fput() is needed in fd.flags, so that
fdput() does the right thing.


Thanks Al, it is quite clear that issue can't happen while a ioctl is in progress.
Actually the issue seems to be a race while glue_dir input is removed.

  114.339374] input: syz1 as /devices/virtual/input/input278
[  114.345619] input: syz1 as /devices/virtual/input/input279
[  114.353502] input: syz1 as /devices/virtual/input/input280
[  114.361907] input: syz1 as /devices/virtual/input/input281
[  114.367276] input: syz1 as /devices/virtual/input/input282
[  114.382292] input: syz1 as /devices/virtual/input/input283

in our case it is input which is getting removed while a inputxx is trying make node inside input.

Similar issue https://lkml.org/lkml/2019/5/1/3

Thanks,
Mukesh






[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux