lkml.org 
[lkml]   [2019]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [BUG] char: pcmcia: a possible concurrency double-free bug in rx_alloc_buffers()
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

\
 
 \ /
  Last update: 2019-11-21 08:08    [W:0.052 / U:1.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site