| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7b041177-04a4-4faf-af0a-c71ddcb1153c@linux.dev>
Date: Tue, 10 Dec 2024 16:44:40 +0100
From: Zhu Yanjun <yanjun.zhu@...ux.dev>
To: Bernard Metzler <bmt@...ich.ibm.com>, linux-rdma@...r.kernel.org
Cc: jgg@...pe.ca, leon@...nel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com,
zyjzyj2000@...il.com, syzbot+4b87489410b4efd181bf@...kaller.appspotmail.com
Subject: Re: [PATCH] RDMA/siw: Remove direct link to net_device
On 10.12.24 14:03, Bernard Metzler wrote:
> Maintain needed network interface information locally, but
> remove a direct link to net_device which can become stale.
> Accessing a stale net_device link was causing a 'KASAN:
> slab-use-after-free' exception during siw_query_port()
> call.
>
> Fixes: bdcf26bf9b3a ("rdma/siw: network and RDMA core interface")
> Reported-by: syzbot+4b87489410b4efd181bf@...kaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=4b87489410b4efd181bf
> Signed-off-by: Bernard Metzler <bmt@...ich.ibm.com>
> ---
> drivers/infiniband/sw/siw/siw.h | 11 +++++++----
> drivers/infiniband/sw/siw/siw_cm.c | 4 ++--
> drivers/infiniband/sw/siw/siw_main.c | 18 ++++++++++++------
> drivers/infiniband/sw/siw/siw_verbs.c | 11 ++++++-----
> 4 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
> index 86d4d6a2170e..c8f75527b513 100644
> --- a/drivers/infiniband/sw/siw/siw.h
> +++ b/drivers/infiniband/sw/siw/siw.h
> @@ -69,16 +69,19 @@ struct siw_pd {
>
> struct siw_device {
> struct ib_device base_dev;
> - struct net_device *netdev;
> struct siw_dev_cap attrs;
>
> u32 vendor_part_id;
> + struct {
> + int ifindex;
> + enum ib_port_state state;
> + enum ib_mtu mtu;
> + enum ib_mtu max_mtu;
> + } ifinfo;
> +
> int numa_node;
> char raw_gid[ETH_ALEN];
>
> - /* physical port state (only one port per device) */
> - enum ib_port_state state;
> -
> spinlock_t lock;
>
> struct xarray qp_xa;
> diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
> index 86323918a570..451b50d92f7f 100644
> --- a/drivers/infiniband/sw/siw/siw_cm.c
> +++ b/drivers/infiniband/sw/siw/siw_cm.c
> @@ -1780,7 +1780,7 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
>
> /* For wildcard addr, limit binding to current device only */
> if (ipv4_is_zeronet(laddr->sin_addr.s_addr))
> - s->sk->sk_bound_dev_if = sdev->netdev->ifindex;
> + s->sk->sk_bound_dev_if = sdev->ifinfo.ifindex;
>
> rv = s->ops->bind(s, (struct sockaddr *)laddr,
> sizeof(struct sockaddr_in));
> @@ -1798,7 +1798,7 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
>
> /* For wildcard addr, limit binding to current device only */
> if (ipv6_addr_any(&laddr->sin6_addr))
> - s->sk->sk_bound_dev_if = sdev->netdev->ifindex;
> + s->sk->sk_bound_dev_if = sdev->ifinfo.ifindex;
>
> rv = s->ops->bind(s, (struct sockaddr *)laddr,
> sizeof(struct sockaddr_in6));
> diff --git a/drivers/infiniband/sw/siw/siw_main.c b/drivers/infiniband/sw/siw/siw_main.c
> index 17abef48abcd..4db10bdfb515 100644
> --- a/drivers/infiniband/sw/siw/siw_main.c
> +++ b/drivers/infiniband/sw/siw/siw_main.c
> @@ -287,7 +287,6 @@ static struct siw_device *siw_device_create(struct net_device *netdev)
> return NULL;
>
> base_dev = &sdev->base_dev;
> - sdev->netdev = netdev;
>
> if (netdev->addr_len) {
> memcpy(sdev->raw_gid, netdev->dev_addr,
> @@ -354,6 +353,10 @@ static struct siw_device *siw_device_create(struct net_device *netdev)
> atomic_set(&sdev->num_mr, 0);
> atomic_set(&sdev->num_pd, 0);
>
> + sdev->ifinfo.max_mtu = ib_mtu_int_to_enum(netdev->max_mtu);
> + sdev->ifinfo.mtu = ib_mtu_int_to_enum(READ_ONCE(netdev->mtu));
> + sdev->ifinfo.ifindex = netdev->ifindex;
> +
> sdev->numa_node = dev_to_node(&netdev->dev);
> spin_lock_init(&sdev->lock);
>
> @@ -381,12 +384,12 @@ static int siw_netdev_event(struct notifier_block *nb, unsigned long event,
>
> switch (event) {
> case NETDEV_UP:
> - sdev->state = IB_PORT_ACTIVE;
> + sdev->ifinfo.state = IB_PORT_ACTIVE;
> siw_port_event(sdev, 1, IB_EVENT_PORT_ACTIVE);
> break;
>
> case NETDEV_DOWN:
> - sdev->state = IB_PORT_DOWN;
> + sdev->ifinfo.state = IB_PORT_DOWN;
> siw_port_event(sdev, 1, IB_EVENT_PORT_ERR);
> break;
>
> @@ -406,10 +409,13 @@ static int siw_netdev_event(struct notifier_block *nb, unsigned long event,
> case NETDEV_CHANGEADDR:
> siw_port_event(sdev, 1, IB_EVENT_LID_CHANGE);
> break;
> +
> + case NETDEV_CHANGEMTU:
> + sdev->ifinfo.mtu = ib_mtu_int_to_enum(READ_ONCE(netdev->mtu));
> + break;
> /*
> * Todo: Below netdev events are currently not handled.
> */
> - case NETDEV_CHANGEMTU:
> case NETDEV_CHANGE:
> break;
>
> @@ -444,9 +450,9 @@ static int siw_newlink(const char *basedev_name, struct net_device *netdev)
> dev_dbg(&netdev->dev, "siw: new device\n");
>
> if (netif_running(netdev) && netif_carrier_ok(netdev))
> - sdev->state = IB_PORT_ACTIVE;
> + sdev->ifinfo.state = IB_PORT_ACTIVE;
> else
> - sdev->state = IB_PORT_DOWN;
> + sdev->ifinfo.state = IB_PORT_DOWN;
>
> ib_mark_name_assigned_by_user(&sdev->base_dev);
> rv = siw_device_register(sdev, basedev_name);
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> index 986666c19378..3ab9c5170637 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -178,14 +178,15 @@ int siw_query_port(struct ib_device *base_dev, u32 port,
>
> rv = ib_get_eth_speed(base_dev, port, &attr->active_speed,
> &attr->active_width);
> +
> attr->gid_tbl_len = 1;
> attr->max_msg_sz = -1;
> - attr->max_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
> - attr->active_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
> - attr->phys_state = sdev->state == IB_PORT_ACTIVE ?
> + attr->max_mtu = sdev->ifinfo.max_mtu;
> + attr->active_mtu = sdev->ifinfo.mtu;
> + attr->phys_state = sdev->ifinfo.state == IB_PORT_ACTIVE ?
> IB_PORT_PHYS_STATE_LINK_UP : IB_PORT_PHYS_STATE_DISABLED;
> attr->port_cap_flags = IB_PORT_CM_SUP | IB_PORT_DEVICE_MGMT_SUP;
> - attr->state = sdev->state;
> + attr->state = sdev->ifinfo.state;
> /*
> * All zero
> *
> @@ -519,7 +520,7 @@ int siw_query_qp(struct ib_qp *base_qp, struct ib_qp_attr *qp_attr,
> qp_attr->cap.max_send_sge = qp->attrs.sq_max_sges;
> qp_attr->cap.max_recv_wr = qp->attrs.rq_size;
> qp_attr->cap.max_recv_sge = qp->attrs.rq_max_sges;
> - qp_attr->path_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
> + qp_attr->path_mtu = sdev->ifinfo.mtu;
The followings are my fix to this kind of problem
https://syzkaller.appspot.com/bug?extid=4b87489410b4efd181bf
It seems that it can also fix this problem.
After I delved into your commit, I wonder what happen if the netdev will
be handled in the future after xxx_query_port?
Thanks,
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c
b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 5c18f7e342f2..7c73eb9115f1 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -57,7 +57,7 @@ static int rxe_query_port(struct ib_device *ibdev,
if (attr->state == IB_PORT_ACTIVE)
attr->phys_state = IB_PORT_PHYS_STATE_LINK_UP;
- else if (dev_get_flags(rxe->ndev) & IFF_UP)
+ else if (rxe && rxe->ndev && (dev_get_flags(rxe->ndev) & IFF_UP))
attr->phys_state = IB_PORT_PHYS_STATE_POLLING;
else
attr->phys_state = IB_PORT_PHYS_STATE_DISABLED;
Zhu Yanjun
> qp_attr->max_rd_atomic = qp->attrs.irq_size;
> qp_attr->max_dest_rd_atomic = qp->attrs.orq_size;
>
Powered by blists - more mailing lists