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

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

[Greg KH]

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.
> 
> This possible bug is found by a static analysis tool written by myself and my manual code review.

Care to send a patch to fix up this potential issue?

thanks,

greg k-h

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

[Greg KH]

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

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

[Jia-Ju Bai]

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

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

[Dominik Brodowski]

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