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?
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?
thanks,
greg k-h
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
On Mon, Jan 07, 2019 at 05:33:43PM +0800, Jia-Ju Bai wrote:
>
>
> 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?
AFAICS, this is a non-issue: If hdlcdev_open() is called, it sets
info->netcount=1. If info->netcount!=0, mgslpc_open() will abort before
calling startup(). And if mgslpc_open() is called, it sets info->count=1,
causing hdlcdev_open() to fail before calling startup(). So no risk of
concurrency here.
Best,
Dominik