From: Dan Carpenter <dan.carpenter@oracle.com>
To: Duoming Zhou <duoming@zju.edu.cn>
Cc: linux-hams@vger.kernel.org, jreuter@yaina.de,
ralf@linux-mips.org, davem@davemloft.net, kuba@kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [Fixes] ax25: add refcount in ax25_dev to avoid UAF bugs
Date: Wed, 2 Feb 2022 20:47:09 +0300 [thread overview]
Message-ID: <20220202174709.GY1978@kadam> (raw)
In-Reply-To: <20220202161846.26198-1-duoming@zju.edu.cn>
Hi Duoming Zhou,
The original patch was already applied so you need to just send the
fixes by themselves on top of the net-next git tree.
> ---
> include/net/ax25.h | 12 ++++++++++++
> net/ax25/af_ax25.c | 20 ++++++++++++++++----
> net/ax25/ax25_dev.c | 33 +++++++++++++++++++++++++++------
> net/ax25/ax25_route.c | 27 ++++++++++++++++++++++-----
> 4 files changed, 77 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/ax25.h b/include/net/ax25.h
> index 526e4958919..1a38b1ad529 100644
> --- a/include/net/ax25.h
> +++ b/include/net/ax25.h
> @@ -239,6 +239,7 @@ typedef struct ax25_dev {
> #if defined(CONFIG_AX25_DAMA_SLAVE) || defined(CONFIG_AX25_DAMA_MASTER)
> ax25_dama_info dama;
> #endif
> + refcount_t refcount;
> } ax25_dev;
>
> typedef struct ax25_cb {
> @@ -293,6 +294,17 @@ static __inline__ void ax25_cb_put(ax25_cb *ax25)
> }
> }
>
> +static inline void ax25_dev_hold(ax25_dev *ax25_dev)
> +{
> + refcount_inc(&ax25_dev->refcount);
> +}
> +
> +static inline void ax25_dev_put(ax25_dev *ax25_dev)
> +{
> + if (refcount_dec_and_test(&ax25_dev->refcount))
> + kfree(ax25_dev);
> +}
> +
Forget about these cleanups for now. Just fix the reference leaks on
error.
> static inline __be16 ax25_type_trans(struct sk_buff *skb, struct net_device *dev)
> {
> skb->dev = dev;
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 44a8730c26a..7463bbd4e63 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -91,6 +91,7 @@ static void ax25_kill_by_device(struct net_device *dev)
> spin_unlock_bh(&ax25_list_lock);
> lock_sock(sk);
> s->ax25_dev = NULL;
> + ax25_dev_put(ax25_dev);
> release_sock(sk);
> ax25_disconnect(s, ENETUNREACH);
> spin_lock_bh(&ax25_list_lock);
> @@ -358,21 +359,31 @@ static int ax25_ctl_ioctl(const unsigned int cmd, void __user *arg)
> if (copy_from_user(&ax25_ctl, arg, sizeof(ax25_ctl)))
> return -EFAULT;
>
> - if ((ax25_dev = ax25_addr_ax25dev(&ax25_ctl.port_addr)) == NULL)
> + ax25_dev = ax25_addr_ax25dev(&ax25_ctl.port_addr);
> + if (ax25_dev == NULL) {
> + ax25_dev_put(ax25_dev);
This one will lead to a NULL dereference. These NULL dereferences are
> return -ENODEV;
> + }
>
> - if (ax25_ctl.digi_count > AX25_MAX_DIGIS)
> + if (ax25_ctl.digi_count > AX25_MAX_DIGIS) {
> + ax25_dev_put(ax25_dev);
> return -EINVAL;
> + }
>
> - if (ax25_ctl.arg > ULONG_MAX / HZ && ax25_ctl.cmd != AX25_KILL)
> + if (ax25_ctl.arg > ULONG_MAX / HZ && ax25_ctl.cmd != AX25_KILL) {
> + ax25_dev_put(ax25_dev);
> return -EINVAL;
> + }
>
> digi.ndigi = ax25_ctl.digi_count;
> for (k = 0; k < digi.ndigi; k++)
> digi.calls[k] = ax25_ctl.digi_addr[k];
>
> - if ((ax25 = ax25_find_cb(&ax25_ctl.source_addr, &ax25_ctl.dest_addr, &digi, ax25_dev->dev)) == NULL)
> + ax25 = ax25_find_cb(&ax25_ctl.source_addr, &ax25_ctl.dest_addr, &digi, ax25_dev->dev);
> + if (ax25 == NULL) {
> + ax25_dev_put(ax25_dev);
Don't do this.
> return -ENOTCONN;
> + }
>
> switch (ax25_ctl.cmd) {
> case AX25_KILL:
> @@ -439,6 +450,7 @@ static int ax25_ctl_ioctl(const unsigned int cmd, void __user *arg)
> }
>
> out_put:
> + ax25_dev_put(ax25_dev);
> ax25_cb_put(ax25);
> return ret;
>
> diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
> index 256fadb94df..77d9aa2ccab 100644
> --- a/net/ax25/ax25_dev.c
> +++ b/net/ax25/ax25_dev.c
> @@ -37,6 +37,7 @@ ax25_dev *ax25_addr_ax25dev(ax25_address *addr)
> for (ax25_dev = ax25_dev_list; ax25_dev != NULL; ax25_dev = ax25_dev->next)
> if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) {
> res = ax25_dev;
> + ax25_dev_hold(ax25_dev);
> }
> spin_unlock_bh(&ax25_dev_lock);
>
> @@ -56,6 +57,7 @@ void ax25_dev_device_up(struct net_device *dev)
> return;
> }
>
> + refcount_set(&ax25_dev->refcount, 1);
> dev->ax25_ptr = ax25_dev;
> ax25_dev->dev = dev;
> dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC);
> @@ -83,6 +85,7 @@ void ax25_dev_device_up(struct net_device *dev)
> spin_lock_bh(&ax25_dev_lock);
> ax25_dev->next = ax25_dev_list;
> ax25_dev_list = ax25_dev;
> + ax25_dev_hold(ax25_dev);
> spin_unlock_bh(&ax25_dev_lock);
>
> ax25_register_dev_sysctl(ax25_dev);
> @@ -111,21 +114,23 @@ void ax25_dev_device_down(struct net_device *dev)
> s->forward = NULL;
>
> if ((s = ax25_dev_list) == ax25_dev) {
> + ax25_dev_put(ax25_dev_list);
Just leave the style changes until later. I'm sorry I brought it up.
> ax25_dev_list = s->next;
> spin_unlock_bh(&ax25_dev_lock);
> dev->ax25_ptr = NULL;
> dev_put_track(dev, &ax25_dev->dev_tracker);
> - kfree(ax25_dev);
> + ax25_dev_put(ax25_dev);
> return;
> }
>
> while (s != NULL && s->next != NULL) {
> if (s->next == ax25_dev) {
> + ax25_dev_put(s->next);
> s->next = ax25_dev->next;
> spin_unlock_bh(&ax25_dev_lock);
> dev->ax25_ptr = NULL;
> dev_put_track(dev, &ax25_dev->dev_tracker);
> - kfree(ax25_dev);
> + ax25_dev_put(ax25_dev);
> return;
> }
>
> @@ -133,31 +138,47 @@ void ax25_dev_device_down(struct net_device *dev)
> }
> spin_unlock_bh(&ax25_dev_lock);
> dev->ax25_ptr = NULL;
> + ax25_dev_put(ax25_dev);
> }
>
> int ax25_fwd_ioctl(unsigned int cmd, struct ax25_fwd_struct *fwd)
> {
> ax25_dev *ax25_dev, *fwd_dev;
>
> - if ((ax25_dev = ax25_addr_ax25dev(&fwd->port_from)) == NULL)
> + ax25_dev = ax25_addr_ax25dev(&fwd->port_from);
> + if (ax25_dev == NULL) {
> + ax25_dev_put(ax25_dev);
NULL dereference.
> return -EINVAL;
> + }
>
> switch (cmd) {
> case SIOCAX25ADDFWD:
> - if ((fwd_dev = ax25_addr_ax25dev(&fwd->port_to)) == NULL)
> + fwd_dev = ax25_addr_ax25dev(&fwd->port_to);
> + if (fwd_dev == NULL) {
> + ax25_dev_put(fwd_dev);
NULL dereference.
> + ax25_dev_put(ax25_dev);
> return -EINVAL;
> - if (ax25_dev->forward != NULL)
> + }
> + if (ax25_dev->forward != NULL) {
> + ax25_dev_put(ax25_dev);
Drop the fwd_dev reference as well.
> return -EINVAL;
> + }
> ax25_dev->forward = fwd_dev->dev;
> + ax25_dev_put(fwd_dev);
> + ax25_dev_put(ax25_dev);
> break;
>
> case SIOCAX25DELFWD:
> - if (ax25_dev->forward == NULL)
> + if (ax25_dev->forward == NULL) {
> + ax25_dev_put(ax25_dev);
> return -EINVAL;
> + }
> ax25_dev->forward = NULL;
> + ax25_dev_put(ax25_dev);
> break;
>
> default:
> + ax25_dev_put(ax25_dev);
> return -EINVAL;
> }
>
> diff --git a/net/ax25/ax25_route.c b/net/ax25/ax25_route.c
> index d0b2e094bd5..7fe7a83b2a3 100644
> --- a/net/ax25/ax25_route.c
> +++ b/net/ax25/ax25_route.c
> @@ -75,10 +75,15 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
> ax25_dev *ax25_dev;
> int i;
>
> - if ((ax25_dev = ax25_addr_ax25dev(&route->port_addr)) == NULL)
> + ax25_dev = ax25_addr_ax25dev(&route->port_addr);
> + if (ax25_dev == NULL) {
> + ax25_dev_put(ax25_dev);
NULL dereference.
> return -EINVAL;
> - if (route->digi_count > AX25_MAX_DIGIS)
> + }
> + if (route->digi_count > AX25_MAX_DIGIS) {
> + ax25_dev_put(ax25_dev);
> return -EINVAL;
> + }
>
> write_lock_bh(&ax25_route_lock);
>
> @@ -91,6 +96,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
> if (route->digi_count != 0) {
> if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_ATOMIC)) == NULL) {
> write_unlock_bh(&ax25_route_lock);
> + ax25_dev_put(ax25_dev);
> return -ENOMEM;
> }
> ax25_rt->digipeat->lastrepeat = -1;
> @@ -101,6 +107,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
> }
> }
> write_unlock_bh(&ax25_route_lock);
> + ax25_dev_put(ax25_dev);
> return 0;
> }
> ax25_rt = ax25_rt->next;
> @@ -108,6 +115,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
>
> if ((ax25_rt = kmalloc(sizeof(ax25_route), GFP_ATOMIC)) == NULL) {
> write_unlock_bh(&ax25_route_lock);
> + ax25_dev_put(ax25_dev);
> return -ENOMEM;
> }
>
> @@ -120,6 +128,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
> if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_ATOMIC)) == NULL) {
> write_unlock_bh(&ax25_route_lock);
> kfree(ax25_rt);
> + ax25_dev_put(ax25_dev);
> return -ENOMEM;
> }
> ax25_rt->digipeat->lastrepeat = -1;
> @@ -132,6 +141,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
> ax25_rt->next = ax25_route_list;
> ax25_route_list = ax25_rt;
> write_unlock_bh(&ax25_route_lock);
> + ax25_dev_put(ax25_dev);
>
> return 0;
> }
> @@ -147,8 +157,11 @@ static int ax25_rt_del(struct ax25_routes_struct *route)
> ax25_route *s, *t, *ax25_rt;
> ax25_dev *ax25_dev;
>
> - if ((ax25_dev = ax25_addr_ax25dev(&route->port_addr)) == NULL)
> + ax25_dev = ax25_addr_ax25dev(&route->port_addr);
> + if (ax25_dev == NULL) {
> + ax25_dev_put(ax25_dev);
NULL dereference.
> return -EINVAL;
> + }
>
> write_lock_bh(&ax25_route_lock);
>
> @@ -173,7 +186,7 @@ static int ax25_rt_del(struct ax25_routes_struct *route)
> }
> }
> write_unlock_bh(&ax25_route_lock);
> -
> + ax25_dev_put(ax25_dev);
> return 0;
> }
>
> @@ -183,8 +196,11 @@ static int ax25_rt_opt(struct ax25_route_opt_struct *rt_option)
> ax25_dev *ax25_dev;
> int err = 0;
>
> - if ((ax25_dev = ax25_addr_ax25dev(&rt_option->port_addr)) == NULL)
> + ax25_dev = ax25_addr_ax25dev(&rt_option->port_addr);
> + if (ax25_dev == NULL) {
> + ax25_dev_put(ax25_dev);
NULL dereference.
> return -EINVAL;
> + }
>
> write_lock_bh(&ax25_route_lock);
>
> @@ -215,6 +231,7 @@ static int ax25_rt_opt(struct ax25_route_opt_struct *rt_option)
>
> out:
> write_unlock_bh(&ax25_route_lock);
> + ax25_dev_put(ax25_dev);
> return err;
> }
Thanks for this, but please can you resend with the above issues fixed
and based on top of net-next.
regards,
dan carpenter
prev parent reply other threads:[~2022-02-02 17:47 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-02 16:18 [Fixes] ax25: add refcount in ax25_dev to avoid UAF bugs Duoming Zhou
2022-02-02 17:47 ` Dan Carpenter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220202174709.GY1978@kadam \
--to=dan.carpenter@oracle.com \
--cc=davem@davemloft.net \
--cc=duoming@zju.edu.cn \
--cc=jreuter@yaina.de \
--cc=kuba@kernel.org \
--cc=linux-hams@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=ralf@linux-mips.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.