Message ID | 20190122184059.220252-1-edumazet@google.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [net] ax25: fix possible use-after-free | expand |
From: Eric Dumazet <edumazet@google.com> Date: Tue, 22 Jan 2019 10:40:59 -0800 > syzbot found that ax25 routes where not properly protected > against concurrent use [1]. > > In this particular report the bug happened while > copying ax25->digipeat. > > Fix this problem by making sure we call ax25_get_route() > while ax25_route_lock is held, so that no modification > could happen while using the route. > > The current two ax25_get_route() callers do not sleep, > so this change should be fine. > > Once we do that, ax25_get_route() no longer needs to > grab a reference on the found route. > > [1] ... > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Ralf Baechle <ralf@linux-mips.org> > Reported-by: syzbot <syzkaller@googlegroups.com> Applied.
On Tue, Jan 22, 2019 at 10:41 AM 'Eric Dumazet' via syzkaller <syzkaller@googlegroups.com> wrote: > > syzbot found that ax25 routes where not properly protected > against concurrent use [1]. > > In this particular report the bug happened while > copying ax25->digipeat. > > Fix this problem by making sure we call ax25_get_route() > while ax25_route_lock is held, so that no modification > could happen while using the route. ax25_route_lock_use() is a read lock, so two ax25_rt_autobind() could still enter the same critical section? > > The current two ax25_get_route() callers do not sleep, > so this change should be fine. > > Once we do that, ax25_get_route() no longer needs to > grab a reference on the found route. . After your patch, ax25_hold_route() has no callers while ax25_put_route() still does. Is ->refcount always 1? Thanks.
On 01/23/2019 03:25 PM, Cong Wang wrote: > On Tue, Jan 22, 2019 at 10:41 AM 'Eric Dumazet' via syzkaller > <syzkaller@googlegroups.com> wrote: >> >> syzbot found that ax25 routes where not properly protected >> against concurrent use [1]. >> >> In this particular report the bug happened while >> copying ax25->digipeat. >> >> Fix this problem by making sure we call ax25_get_route() >> while ax25_route_lock is held, so that no modification >> could happen while using the route. > > ax25_route_lock_use() is a read lock, so two ax25_rt_autobind() > could still enter the same critical section? > Not sure I understand your concern. The two ax25_rt_autobind() would only read the route contents, so that should be fine ? > >> >> The current two ax25_get_route() callers do not sleep, >> so this change should be fine. >> >> Once we do that, ax25_get_route() no longer needs to >> grab a reference on the found route. > . > > After your patch, ax25_hold_route() has no callers while > ax25_put_route() still does. Is ->refcount always 1? Yes, the plan is to remove this refcount in net-next.
On Wed, Jan 23, 2019 at 3:42 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 01/23/2019 03:25 PM, Cong Wang wrote: > > On Tue, Jan 22, 2019 at 10:41 AM 'Eric Dumazet' via syzkaller > > <syzkaller@googlegroups.com> wrote: > >> > >> syzbot found that ax25 routes where not properly protected > >> against concurrent use [1]. > >> > >> In this particular report the bug happened while > >> copying ax25->digipeat. > >> > >> Fix this problem by making sure we call ax25_get_route() > >> while ax25_route_lock is held, so that no modification > >> could happen while using the route. > > > > ax25_route_lock_use() is a read lock, so two ax25_rt_autobind() > > could still enter the same critical section? > > > > Not sure I understand your concern. > > The two ax25_rt_autobind() would only read the route contents, > so that should be fine ? Not sure if it is read-only and safe for the following code: if (ax25_rt->digipeat != NULL) { ax25->digipeat = kmemdup(ax25_rt->digipeat, sizeof(ax25_digi), GFP_ATOMIC); if (ax25->digipeat == NULL) { err = -ENOMEM; goto put; } ax25_adjust_path(addr, ax25->digipeat); } Maybe we leak memory here at least, not sure... > > > > >> > >> The current two ax25_get_route() callers do not sleep, > >> so this change should be fine. > >> > >> Once we do that, ax25_get_route() no longer needs to > >> grab a reference on the found route. > > . > > > > After your patch, ax25_hold_route() has no callers while > > ax25_put_route() still does. Is ->refcount always 1? > > Yes, the plan is to remove this refcount in net-next. > Good to know. Thanks.
On Wed, Jan 23, 2019 at 5:12 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Wed, Jan 23, 2019 at 3:42 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > > > > > On 01/23/2019 03:25 PM, Cong Wang wrote: > > > On Tue, Jan 22, 2019 at 10:41 AM 'Eric Dumazet' via syzkaller > > > <syzkaller@googlegroups.com> wrote: > > >> > > >> syzbot found that ax25 routes where not properly protected > > >> against concurrent use [1]. > > >> > > >> In this particular report the bug happened while > > >> copying ax25->digipeat. > > >> > > >> Fix this problem by making sure we call ax25_get_route() > > >> while ax25_route_lock is held, so that no modification > > >> could happen while using the route. > > > > > > ax25_route_lock_use() is a read lock, so two ax25_rt_autobind() > > > could still enter the same critical section? > > > > > > > Not sure I understand your concern. > > > > The two ax25_rt_autobind() would only read the route contents, > > so that should be fine ? > > Not sure if it is read-only and safe for the following code: > > if (ax25_rt->digipeat != NULL) { > ax25->digipeat = kmemdup(ax25_rt->digipeat, sizeof(ax25_digi), > GFP_ATOMIC); ax25_rt would be the shared object. (protected by the read lock) ax25 is private object in the thread/socket context. So no worries. > if (ax25->digipeat == NULL) { > err = -ENOMEM; > goto put; > } > ax25_adjust_path(addr, ax25->digipeat); > } > > Maybe we leak memory here at least, not sure... > > > > > > > > > >> > > >> The current two ax25_get_route() callers do not sleep, > > >> so this change should be fine. > > >> > > >> Once we do that, ax25_get_route() no longer needs to > > >> grab a reference on the found route. > > > . > > > > > > After your patch, ax25_hold_route() has no callers while > > > ax25_put_route() still does. Is ->refcount always 1? > > > > Yes, the plan is to remove this refcount in net-next. > > > > Good to know. > > Thanks.
diff --git a/include/net/ax25.h b/include/net/ax25.h index 3f9aea8087e3c823cdd5a22530ed4b25dd7621e1..8b7eb46ad72d8804c1ffaa3943bb2816113239d8 100644 --- a/include/net/ax25.h +++ b/include/net/ax25.h @@ -201,6 +201,18 @@ static inline void ax25_hold_route(ax25_route *ax25_rt) void __ax25_put_route(ax25_route *ax25_rt); +extern rwlock_t ax25_route_lock; + +static inline void ax25_route_lock_use(void) +{ + read_lock(&ax25_route_lock); +} + +static inline void ax25_route_lock_unuse(void) +{ + read_unlock(&ax25_route_lock); +} + static inline void ax25_put_route(ax25_route *ax25_rt) { if (refcount_dec_and_test(&ax25_rt->refcount)) diff --git a/net/ax25/ax25_ip.c b/net/ax25/ax25_ip.c index 70417e9b932ddcc2d7e5eb8ff1652a0d6ae6d457..314bbc8010fbedaa779d8b3eb772cc5a0fb26eda 100644 --- a/net/ax25/ax25_ip.c +++ b/net/ax25/ax25_ip.c @@ -114,6 +114,7 @@ netdev_tx_t ax25_ip_xmit(struct sk_buff *skb) dst = (ax25_address *)(bp + 1); src = (ax25_address *)(bp + 8); + ax25_route_lock_use(); route = ax25_get_route(dst, NULL); if (route) { digipeat = route->digipeat; @@ -206,9 +207,8 @@ netdev_tx_t ax25_ip_xmit(struct sk_buff *skb) ax25_queue_xmit(skb, dev); put: - if (route) - ax25_put_route(route); + ax25_route_lock_unuse(); return NETDEV_TX_OK; } diff --git a/net/ax25/ax25_route.c b/net/ax25/ax25_route.c index a0eff323af12c027ea13a70bfbfffa68b5e48324..66f74c85cf6bd1487a13fbfeeca9eabcdf58fe11 100644 --- a/net/ax25/ax25_route.c +++ b/net/ax25/ax25_route.c @@ -40,7 +40,7 @@ #include <linux/export.h> static ax25_route *ax25_route_list; -static DEFINE_RWLOCK(ax25_route_lock); +DEFINE_RWLOCK(ax25_route_lock); void ax25_rt_device_down(struct net_device *dev) { @@ -335,6 +335,7 @@ const struct seq_operations ax25_rt_seqops = { * Find AX.25 route * * Only routes with a reference count of zero can be destroyed. + * Must be called with ax25_route_lock read locked. */ ax25_route *ax25_get_route(ax25_address *addr, struct net_device *dev) { @@ -342,7 +343,6 @@ ax25_route *ax25_get_route(ax25_address *addr, struct net_device *dev) ax25_route *ax25_def_rt = NULL; ax25_route *ax25_rt; - read_lock(&ax25_route_lock); /* * Bind to the physical interface we heard them on, or the default * route if none is found; @@ -365,11 +365,6 @@ ax25_route *ax25_get_route(ax25_address *addr, struct net_device *dev) if (ax25_spe_rt != NULL) ax25_rt = ax25_spe_rt; - if (ax25_rt != NULL) - ax25_hold_route(ax25_rt); - - read_unlock(&ax25_route_lock); - return ax25_rt; } @@ -400,9 +395,12 @@ int ax25_rt_autobind(ax25_cb *ax25, ax25_address *addr) ax25_route *ax25_rt; int err = 0; - if ((ax25_rt = ax25_get_route(addr, NULL)) == NULL) + ax25_route_lock_use(); + ax25_rt = ax25_get_route(addr, NULL); + if (!ax25_rt) { + ax25_route_lock_unuse(); return -EHOSTUNREACH; - + } if ((ax25->ax25_dev = ax25_dev_ax25dev(ax25_rt->dev)) == NULL) { err = -EHOSTUNREACH; goto put; @@ -437,8 +435,7 @@ int ax25_rt_autobind(ax25_cb *ax25, ax25_address *addr) } put: - ax25_put_route(ax25_rt); - + ax25_route_lock_unuse(); return err; }