Re: [BUG] infiniband: mlx5: a possible null-pointer dereference in set_roce_addr()
On Mon, Jul 29, 2019 at 05:26:30AM +, Parav Pandit wrote:
>
>
> > -Original Message-
> > From: linux-kernel-ow...@vger.kernel.org > ow...@vger.kernel.org> On Behalf Of Parav Pandit
> > Sent: Monday, July 29, 2019 10:55 AM
> > To: Jia-Ju Bai ; l...@kernel.org;
> > dledf...@redhat.com; j...@ziepe.ca
> > Cc: linux-r...@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: RE: [BUG] infiniband: mlx5: a possible null-pointer dereference in
> > set_roce_addr()
> >
> > Hi Jia,
> >
> > > -Original Message-
> > > From: linux-rdma-ow...@vger.kernel.org > > ow...@vger.kernel.org> On Behalf Of Jia-Ju Bai
> > > Sent: Monday, July 29, 2019 7:47 AM
> > > To: l...@kernel.org; dledf...@redhat.com; j...@ziepe.ca
> > > Cc: linux-r...@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: [BUG] infiniband: mlx5: a possible null-pointer dereference
> > > in
> > > set_roce_addr()
> > >
> > > In set_roce_addr(), there is an if statement on line 589 to check
> > > whether gid is
> > > NULL:
> > > if (gid)
> > >
> > > When gid is NULL, it is used on line 613:
> > > return mlx5_core_roce_gid_set(..., gid->raw, ...);
> > >
> > > Thus, a possible null-pointer dereference may occur.
> > >
> > > This bug is found by a static analysis tool STCheck written by us.
> > >
> > While static checker is right, it is not a real bug, because gid->raw
> > pointer
> > points to GID entry itself so when GID is NULL, gid->raw is NULL too.
> >
> > One way to suppress the static checker warning/error is below patch.
> > Will let Leon review it.
> >
> > > I do not know how to correctly fix this bug, so I only report it.
> > >
> > >
> > > Best wishes,
> > > Jia-Ju Bai
> >
> > From 30e055dba77e595bf88aebd3a9c75ed76bc9c65a Mon Sep 17 00:00:00
> > 2001
> > From: Parav Pandit
> > Date: Mon, 29 Jul 2019 00:13:21 -0500
> > Subject: [PATCH] IB/mlx5: Avoid static checker warning for NULL access
> >
> > union ib_gid *gid and gid->raw pointers refers to the same address.
> > However some static checker reports this as possible NULL access warning in
> > call to mlx5_core_roce_gid_set().
> >
> > To suppress such warning, instead of working on raw GID element, expose API
> > using union ib_gid*.
> >
> > Reported-by: Jia-Ju Bai
> > Signed-off-by: Parav Pandit
> > ---
> > drivers/infiniband/hw/mlx5/main.c | 2 +-
> > drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c | 12 +++-
> > drivers/net/ethernet/mellanox/mlx5/core/lib/gid.c | 5 +++--
> > drivers/net/ethernet/mellanox/mlx5/core/rdma.c | 2 +-
> > include/linux/mlx5/driver.h | 4 +++-
> > 5 files changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx5/main.c
> > b/drivers/infiniband/hw/mlx5/main.c
> > index c2a5780cb394..e60785bad7ef 100644
> > --- a/drivers/infiniband/hw/mlx5/main.c
> > +++ b/drivers/infiniband/hw/mlx5/main.c
> > @@ -610,7 +610,7 @@ static int set_roce_addr(struct mlx5_ib_dev *dev, u8
> > port_num,
> > }
> >
> > return mlx5_core_roce_gid_set(dev->mdev, index, roce_version,
> > - roce_l3_type, gid->raw, mac,
> > + roce_l3_type, , mac,
> > vlan_id < VLAN_CFI_MASK, vlan_id,
> > port_num);
> > }
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> > index 4c50efe4e7f1..76b8236af9c7 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> > @@ -850,6 +850,7 @@ struct mlx5_fpga_conn *mlx5_fpga_conn_create(struct
> > mlx5_fpga_device *fdev,
> > enum mlx5_ifc_fpga_qp_type
> > qp_type) {
> > struct mlx5_fpga_conn *ret, *conn;
> > + struct ib_gid remote_gid = {};
> > u8 *remote_mac, *remote_ip;
> > int err;
> >
> > @@ -876,11 +877,12 @@ struct mlx5_fpga_conn
> > *mlx5_fpga_conn_create(struct mlx5_fpga_device *fdev,
> > goto err;
> > }
> >
> > - /* Build Modified EUI-64 IPv6 address from the MAC address */
> > remote_ip = MLX5
RE: [BUG] infiniband: mlx5: a possible null-pointer dereference in set_roce_addr()
> -Original Message-
> From: linux-kernel-ow...@vger.kernel.org ow...@vger.kernel.org> On Behalf Of Parav Pandit
> Sent: Monday, July 29, 2019 10:55 AM
> To: Jia-Ju Bai ; l...@kernel.org;
> dledf...@redhat.com; j...@ziepe.ca
> Cc: linux-r...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [BUG] infiniband: mlx5: a possible null-pointer dereference in
> set_roce_addr()
>
> Hi Jia,
>
> > -Original Message-
> > From: linux-rdma-ow...@vger.kernel.org > ow...@vger.kernel.org> On Behalf Of Jia-Ju Bai
> > Sent: Monday, July 29, 2019 7:47 AM
> > To: l...@kernel.org; dledf...@redhat.com; j...@ziepe.ca
> > Cc: linux-r...@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: [BUG] infiniband: mlx5: a possible null-pointer dereference
> > in
> > set_roce_addr()
> >
> > In set_roce_addr(), there is an if statement on line 589 to check
> > whether gid is
> > NULL:
> > if (gid)
> >
> > When gid is NULL, it is used on line 613:
> > return mlx5_core_roce_gid_set(..., gid->raw, ...);
> >
> > Thus, a possible null-pointer dereference may occur.
> >
> > This bug is found by a static analysis tool STCheck written by us.
> >
> While static checker is right, it is not a real bug, because gid->raw pointer
> points to GID entry itself so when GID is NULL, gid->raw is NULL too.
>
> One way to suppress the static checker warning/error is below patch.
> Will let Leon review it.
>
> > I do not know how to correctly fix this bug, so I only report it.
> >
> >
> > Best wishes,
> > Jia-Ju Bai
>
> From 30e055dba77e595bf88aebd3a9c75ed76bc9c65a Mon Sep 17 00:00:00
> 2001
> From: Parav Pandit
> Date: Mon, 29 Jul 2019 00:13:21 -0500
> Subject: [PATCH] IB/mlx5: Avoid static checker warning for NULL access
>
> union ib_gid *gid and gid->raw pointers refers to the same address.
> However some static checker reports this as possible NULL access warning in
> call to mlx5_core_roce_gid_set().
>
> To suppress such warning, instead of working on raw GID element, expose API
> using union ib_gid*.
>
> Reported-by: Jia-Ju Bai
> Signed-off-by: Parav Pandit
> ---
> drivers/infiniband/hw/mlx5/main.c | 2 +-
> drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c | 12 +++-
> drivers/net/ethernet/mellanox/mlx5/core/lib/gid.c | 5 +++--
> drivers/net/ethernet/mellanox/mlx5/core/rdma.c | 2 +-
> include/linux/mlx5/driver.h | 4 +++-
> 5 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/main.c
> b/drivers/infiniband/hw/mlx5/main.c
> index c2a5780cb394..e60785bad7ef 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -610,7 +610,7 @@ static int set_roce_addr(struct mlx5_ib_dev *dev, u8
> port_num,
> }
>
> return mlx5_core_roce_gid_set(dev->mdev, index, roce_version,
> - roce_l3_type, gid->raw, mac,
> + roce_l3_type, , mac,
> vlan_id < VLAN_CFI_MASK, vlan_id,
> port_num);
> }
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> b/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> index 4c50efe4e7f1..76b8236af9c7 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> @@ -850,6 +850,7 @@ struct mlx5_fpga_conn *mlx5_fpga_conn_create(struct
> mlx5_fpga_device *fdev,
>enum mlx5_ifc_fpga_qp_type
> qp_type) {
> struct mlx5_fpga_conn *ret, *conn;
> + struct ib_gid remote_gid = {};
> u8 *remote_mac, *remote_ip;
> int err;
>
> @@ -876,11 +877,12 @@ struct mlx5_fpga_conn
> *mlx5_fpga_conn_create(struct mlx5_fpga_device *fdev,
> goto err;
> }
>
> - /* Build Modified EUI-64 IPv6 address from the MAC address */
> remote_ip = MLX5_ADDR_OF(fpga_qpc, conn->fpga_qpc, remote_ip);
> - remote_ip[0] = 0xfe;
> - remote_ip[1] = 0x80;
> - addrconf_addr_eui48(_ip[8], remote_mac);
> + memcpy(remote_gid.raw[0], remote_ip, sizeof(remote_gid.raw));
> + /* Build Modified EUI-64 IPv6 address from the MAC address */
> + remte_gid.raw[0] = 0xfe;
> + remte_gid.raw[1] = 0x80;
> + addrconf_addr_eui48(_gid.raw[8], remote_mac);
>
> err = mlx5_core_reserved_gid_alloc(fdev->mdev,
> >qp.sgid_index);
> if (err) {
> @@ -892,7 +894,7 @@ struct m
RE: [BUG] infiniband: mlx5: a possible null-pointer dereference in set_roce_addr()
Hi Jia,
> -Original Message-
> From: linux-rdma-ow...@vger.kernel.org ow...@vger.kernel.org> On Behalf Of Jia-Ju Bai
> Sent: Monday, July 29, 2019 7:47 AM
> To: l...@kernel.org; dledf...@redhat.com; j...@ziepe.ca
> Cc: linux-r...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [BUG] infiniband: mlx5: a possible null-pointer dereference in
> set_roce_addr()
>
> In set_roce_addr(), there is an if statement on line 589 to check whether gid
> is
> NULL:
> if (gid)
>
> When gid is NULL, it is used on line 613:
> return mlx5_core_roce_gid_set(..., gid->raw, ...);
>
> Thus, a possible null-pointer dereference may occur.
>
> This bug is found by a static analysis tool STCheck written by us.
>
While static checker is right, it is not a real bug, because gid->raw pointer
points to GID entry itself so when GID is NULL, gid->raw is NULL too.
One way to suppress the static checker warning/error is below patch.
Will let Leon review it.
> I do not know how to correctly fix this bug, so I only report it.
>
>
> Best wishes,
> Jia-Ju Bai
From 30e055dba77e595bf88aebd3a9c75ed76bc9c65a Mon Sep 17 00:00:00 2001
From: Parav Pandit
Date: Mon, 29 Jul 2019 00:13:21 -0500
Subject: [PATCH] IB/mlx5: Avoid static checker warning for NULL access
union ib_gid *gid and gid->raw pointers refers to the same address.
However some static checker reports this as possible NULL access
warning in call to mlx5_core_roce_gid_set().
To suppress such warning, instead of working on raw GID element,
expose API using union ib_gid*.
Reported-by: Jia-Ju Bai
Signed-off-by: Parav Pandit
---
drivers/infiniband/hw/mlx5/main.c | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c | 12 +++-
drivers/net/ethernet/mellanox/mlx5/core/lib/gid.c | 5 +++--
drivers/net/ethernet/mellanox/mlx5/core/rdma.c | 2 +-
include/linux/mlx5/driver.h | 4 +++-
5 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/infiniband/hw/mlx5/main.c
b/drivers/infiniband/hw/mlx5/main.c
index c2a5780cb394..e60785bad7ef 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -610,7 +610,7 @@ static int set_roce_addr(struct mlx5_ib_dev *dev, u8
port_num,
}
return mlx5_core_roce_gid_set(dev->mdev, index, roce_version,
- roce_l3_type, gid->raw, mac,
+ roce_l3_type, , mac,
vlan_id < VLAN_CFI_MASK, vlan_id,
port_num);
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
b/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
index 4c50efe4e7f1..76b8236af9c7 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
@@ -850,6 +850,7 @@ struct mlx5_fpga_conn *mlx5_fpga_conn_create(struct
mlx5_fpga_device *fdev,
enum mlx5_ifc_fpga_qp_type qp_type)
{
struct mlx5_fpga_conn *ret, *conn;
+ struct ib_gid remote_gid = {};
u8 *remote_mac, *remote_ip;
int err;
@@ -876,11 +877,12 @@ struct mlx5_fpga_conn *mlx5_fpga_conn_create(struct
mlx5_fpga_device *fdev,
goto err;
}
- /* Build Modified EUI-64 IPv6 address from the MAC address */
remote_ip = MLX5_ADDR_OF(fpga_qpc, conn->fpga_qpc, remote_ip);
- remote_ip[0] = 0xfe;
- remote_ip[1] = 0x80;
- addrconf_addr_eui48(_ip[8], remote_mac);
+ memcpy(remote_gid.raw[0], remote_ip, sizeof(remote_gid.raw));
+ /* Build Modified EUI-64 IPv6 address from the MAC address */
+ remte_gid.raw[0] = 0xfe;
+ remte_gid.raw[1] = 0x80;
+ addrconf_addr_eui48(_gid.raw[8], remote_mac);
err = mlx5_core_reserved_gid_alloc(fdev->mdev, >qp.sgid_index);
if (err) {
@@ -892,7 +894,7 @@ struct mlx5_fpga_conn *mlx5_fpga_conn_create(struct
mlx5_fpga_device *fdev,
err = mlx5_core_roce_gid_set(fdev->mdev, conn->qp.sgid_index,
MLX5_ROCE_VERSION_2,
MLX5_ROCE_L3_TYPE_IPV6,
-remote_ip, remote_mac, true, 0,
+_gid, remote_mac, true, 0,
MLX5_FPGA_PORT_NUM);
if (err) {
mlx5_fpga_err(fdev, "Failed to set SGID: %d\n", err);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/gid.c
b/drivers/net/ethernet/mellanox/mlx5/core/lib/gid.c
index 7722a3f9bb68..9b8563a2bd50 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/gid.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/gid.c
@@ -120,7 +120,8 @@ unsigned int mlx5_core_reserved_gids_count(struct
mlx5_core_dev *dev)
E
Re: [BUG] infiniband: mlx5: a possible null-pointer dereference in set_roce_addr()
On Mon, Jul 29, 2019 at 10:16:30AM +0800, Jia-Ju Bai wrote: > In set_roce_addr(), there is an if statement on line 589 to check whether > gid is NULL: > if (gid) > > When gid is NULL, it is used on line 613: > return mlx5_core_roce_gid_set(..., gid->raw, ...); > > Thus, a possible null-pointer dereference may occur. > > This bug is found by a static analysis tool STCheck written by us. > > I do not know how to correctly fix this bug, so I only report it. You should fix the tool, gid and gid->raw are the same pointers in C. In this case, "mlx5_core_roce_gid_set(..., gid->raw, ...);" will be equal to "mlx5_core_roce_gid_set(..., NULL, ...);" and mlx5_core_roce_gid_set() is designed to handle this case. Thanks > > > Best wishes, > Jia-Ju Bai
[BUG] infiniband: mlx5: a possible null-pointer dereference in set_roce_addr()
In set_roce_addr(), there is an if statement on line 589 to check whether gid is NULL: if (gid) When gid is NULL, it is used on line 613: return mlx5_core_roce_gid_set(..., gid->raw, ...); Thus, a possible null-pointer dereference may occur. This bug is found by a static analysis tool STCheck written by us. I do not know how to correctly fix this bug, so I only report it. Best wishes, Jia-Ju Bai
