Re: [BUG] char: pcmcia: a possible concurrency double-free bug in rx_alloc_buffers()

[Jia-Ju Bai <baijiaju1990@gmail.com>]

On 2019/1/7 16:57, Greg KH wrote:
> On Mon, Jan 07, 2019 at 04:12:22PM +0800, Jia-Ju Bai wrote:
>> In drivers/char/pcmcia/synclink_cs.c, the functions mgslpc_open() and hdlcdev_open() can be concurrently executed.
>>
>> hdlcdev_open
>>    startup
>>      claim_resources
>>        rx_alloc_buffers
>>          line 2641: kfree(info->rx_buf)
>>
>> mgslpc_open
>>    startup
>>      claim_resources
>>        rx_alloc_buffers
>>          line 2641: kfree(info->rx_buf)
>>
>> Thus, a possible concurrency double-free bug may occur.
> Wait, are you sure those really are the same structure, and that those
> two functions can be called at the same time?  That is a tty and a
> network device, are they both created at the same time or does opening
> one create the other?

hdlcdev_open() is assigned to "hdlcdev_ops.ndo_open".
mgslpc_open() is assigned to "mgslpc_ops.open".
They are indeed assigned to the fields in different data structures.

**** For hdlcdev_open() ****
In hdlcdev_init():
     dev->netdev_ops = &hdlcdev_ops;
     rc = register_hdlc_device(dev);
Thus, hdlcdev_open() can be called after "register_hdlc_device(dev)".

hdlcdev_init() is called by mgslpc_add_device(), which is called by 
mgslpc_probe().
mgslpc_probe() is assigned to "mgslpc_driver.probe".

In synclink_cs_init():
     rc = pcmcia_register_driver(&mgslpc_driver);
Thus, mgslpc_probe() can be called after 
"pcmcia_register_driver(&mgslpc_driver)".

As a result, hdlcdev_open() can be executed in synclink_cs_init().

**** For mgslpc_open() ****
In synclink_cs_init():
     tty_set_operations(serial_driver, &mgslpc_ops);
     rc = tty_register_driver(serial_driver);
Thus, mgslpc_open() can be called after 
"tty_register_driver(serial_driver)".

As a result, mgslpc_open() can be executed in synclink_cs_init().

**** For hdlcdev_open() and mgslpc_open() ****
Because mgslpc_open() and hdlcdev_open() can be both executed in 
synclink_cs_init(), I think they can be concurrently executed.


>
> It's not obvious in looking at the code if this really is the same
> structure or not, how did your tool figure it out?

My tool uses the data structure field "info->rx_buf" in the code, so it 
cannot very accurately figure it out.

According to my code review, hdlcdev_open() and mgslpc_open() both call 
"startup(info, tty)", and rx_alloc_buffers() calls kfree(info->rx_buf).
Thus, an important thing is that whether the variable "info" in 
hdlcdev_open() and mgslpc_open() can be the same?
I find this code in hdlcdev_open():
     /* arbitrate between network and tty opens */
     spin_lock_irqsave(&info->netlock, flags);

Thus, the variable "info" in hdlcdev_open() and mgslpc_open() can be the 
same, and "info->rx_buf" in the two calls to kfree() can be the same.

To fix this bug, I think we can reuse the spinlock "info->netlock" to 
protect the function startup() in hdlcdev_open() and mgslpc_open().
But in rx_alloc_buffers(), there are kmalloc(GFP_KERNEL) and 
kzalloc(GFP_KERNEL).
If we reuse the spinlock, we also need to change GFP_KERNEL to GFP_ATOMIC.
What is your opinion?


Best wishes,
Jia-Ju Bai