| Message ID | 20190319214244.20212-1-pakki001@umn.edu |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v4] net: mlx5: Add a missing check on idr_find, free buf | expand |
On Tue, 2019-03-19 at 16:42 -0500, Aditya Pakki wrote: > idr_find() can return a NULL value to 'flow' which is used without a > check. The patch adds a check to avoid potential NULL pointer > dereference. > > In case of mlx5_fpga_sbu_conn_sendmsg() failure, free buf allocated > using kzalloc. > > Fixes: ab412e1dd7db ("net/mlx5: Accel, add TLS rx offload routines") > --- > v3: Reorder buf allocations and flow check. > v2: failure to return in case of flow failure. > v1: Failed to free buf in case of flow failure. > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > --- > drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 14 +++++++++++- > -- > 1 file changed, 11 insertions(+), 3 deletions(-) > Applied to net-mlx5, will be sent upstream shortly, You messed up the Signed-off tag, it also should have been part of the commit message, but don't worry, i fixed it up and added Yuval's Reviewed-by. Thanks Adeitya for handling all of the comments. Saeed.
On 03/19/2019 02:42 PM, Aditya Pakki wrote: > idr_find() can return a NULL value to 'flow' which is used without a > check. The patch adds a check to avoid potential NULL pointer dereference. > > In case of mlx5_fpga_sbu_conn_sendmsg() failure, free buf allocated > using kzalloc. > > Fixes: ab412e1dd7db ("net/mlx5: Accel, add TLS rx offload routines") > --- > v3: Reorder buf allocations and flow check. > v2: failure to return in case of flow failure. > v1: Failed to free buf in case of flow failure. > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > --- > drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c > index 5cf5f2a9d51f..8de64e88c670 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c > @@ -217,15 +217,21 @@ int mlx5_fpga_tls_resync_rx(struct mlx5_core_dev *mdev, u32 handle, u32 seq, > void *cmd; > int ret; > > + rcu_read_lock(); > + flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle)); > + rcu_read_unlock(); This looks suspect (even before your patch) What prevents flow from disappearing after this rcu_read_lock() ? IMO your patch might prevent a NULL deref, but not use-after-free. > + > + if (!flow) { > + WARN_ONCE(1, "Received NULL pointer for handle\n"); > + return -EINVAL; > + } > + > buf = kzalloc(size, GFP_ATOMIC); > if (!buf) > return -ENOMEM; > > cmd = (buf + 1); > > - rcu_read_lock(); > - flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle)); > - rcu_read_unlock(); > mlx5_fpga_tls_flow_to_cmd(flow, cmd); > > MLX5_SET(tls_cmd, cmd, swid, ntohl(handle)); > @@ -238,6 +244,8 @@ int mlx5_fpga_tls_resync_rx(struct mlx5_core_dev *mdev, u32 handle, u32 seq, > buf->complete = mlx_tls_kfree_complete; > > ret = mlx5_fpga_sbu_conn_sendmsg(mdev->fpga->tls->conn, buf); > + if (ret < 0) > + kfree(buf); > > return ret; > } >
On Tue, 2019-03-19 at 21:54 -0700, Eric Dumazet wrote: > > On 03/19/2019 02:42 PM, Aditya Pakki wrote: > > idr_find() can return a NULL value to 'flow' which is used without > > a > > check. The patch adds a check to avoid potential NULL pointer > > dereference. > > > > In case of mlx5_fpga_sbu_conn_sendmsg() failure, free buf allocated > > using kzalloc. > > > > Fixes: ab412e1dd7db ("net/mlx5: Accel, add TLS rx offload > > routines") > > --- > > v3: Reorder buf allocations and flow check. > > v2: failure to return in case of flow failure. > > v1: Failed to free buf in case of flow failure. > > > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > > --- > > drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 14 > > +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c > > b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c > > index 5cf5f2a9d51f..8de64e88c670 100644 > > --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c > > @@ -217,15 +217,21 @@ int mlx5_fpga_tls_resync_rx(struct > > mlx5_core_dev *mdev, u32 handle, u32 seq, > > void *cmd; > > int ret; > > > > + rcu_read_lock(); > > + flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle)); > > + rcu_read_unlock(); > > This looks suspect (even before your patch) > > What prevents flow from disappearing after this rcu_read_lock() ? > > IMO your patch might prevent a NULL deref, but not use-after-free. That crossed my mind when i reviewed this patch but since this is an old issue i just put a todo aside to handle it later i think the author didn't want to deal with use-after-free here, but only wanted to keep the data structure safe against add/remove operations. Anyway all we need here is rcu_read_lock() flow = idr_find(..) mlx5_fpga_tls_flow_to_cmd(flow, cmd); rcu_read_unlock() Will fix it up in a follow up patch, Thank you Eric !! > > > + > > + if (!flow) { > > + WARN_ONCE(1, "Received NULL pointer for handle\n"); > > + return -EINVAL; > > + } > > + > > buf = kzalloc(size, GFP_ATOMIC); > > if (!buf) > > return -ENOMEM; > > > > cmd = (buf + 1); > > > > - rcu_read_lock(); > > - flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle)); > > - rcu_read_unlock(); > > mlx5_fpga_tls_flow_to_cmd(flow, cmd); > > > > MLX5_SET(tls_cmd, cmd, swid, ntohl(handle)); > > @@ -238,6 +244,8 @@ int mlx5_fpga_tls_resync_rx(struct > > mlx5_core_dev *mdev, u32 handle, u32 seq, > > buf->complete = mlx_tls_kfree_complete; > > > > ret = mlx5_fpga_sbu_conn_sendmsg(mdev->fpga->tls->conn, buf); > > + if (ret < 0) > > + kfree(buf); > > > > return ret; > > } > >
On 3/20/2019 7:02 AM, Saeed Mahameed wrote: > On Tue, 2019-03-19 at 21:54 -0700, Eric Dumazet wrote: >> >> On 03/19/2019 02:42 PM, Aditya Pakki wrote: >>> idr_find() can return a NULL value to 'flow' which is used without >>> a >>> check. The patch adds a check to avoid potential NULL pointer >>> dereference. >>> >>> In case of mlx5_fpga_sbu_conn_sendmsg() failure, free buf allocated >>> using kzalloc. >>> >>> Fixes: ab412e1dd7db ("net/mlx5: Accel, add TLS rx offload >>> routines") >>> --- >>> v3: Reorder buf allocations and flow check. >>> v2: failure to return in case of flow failure. >>> v1: Failed to free buf in case of flow failure. >>> >>> Signed-off-by: Aditya Pakki <pakki001@umn.edu> >>> --- >>> drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 14 >>> +++++++++++--- >>> 1 file changed, 11 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c >>> b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c >>> index 5cf5f2a9d51f..8de64e88c670 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c >>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c >>> @@ -217,15 +217,21 @@ int mlx5_fpga_tls_resync_rx(struct >>> mlx5_core_dev *mdev, u32 handle, u32 seq, >>> void *cmd; >>> int ret; >>> >>> + rcu_read_lock(); >>> + flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle)); >>> + rcu_read_unlock(); >> >> This looks suspect (even before your patch) >> >> What prevents flow from disappearing after this rcu_read_lock() ? >> >> IMO your patch might prevent a NULL deref, but not use-after-free. > > That crossed my mind when i reviewed this patch but since this is an > old issue i just put a todo aside to handle it later > > i think the author didn't want to deal with use-after-free here, but > only wanted to keep the data structure safe against add/remove > operations. > > Anyway all we need here is > > rcu_read_lock() > flow = idr_find(..) > mlx5_fpga_tls_flow_to_cmd(flow, cmd); > rcu_read_unlock() > > Will fix it up in a follow up patch, > > Thank you Eric !! > This flow won't be removed unless the tcp connection call sk_destruct or the netdev is removed. Thus, this lookup should *always* succeed. This, is also why I wanted to ask Aditya if this flow was actually encountered in practice. I'm sure that if our assumption about the safety of this flow breaks, then other parts of the TLS driver will fail as well. > >> >>> + >>> + if (!flow) { >>> + WARN_ONCE(1, "Received NULL pointer for handle\n"); >>> + return -EINVAL; >>> + } >>> + >>> buf = kzalloc(size, GFP_ATOMIC); >>> if (!buf) >>> return -ENOMEM; >>> >>> cmd = (buf + 1); >>> >>> - rcu_read_lock(); >>> - flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle)); >>> - rcu_read_unlock(); >>> mlx5_fpga_tls_flow_to_cmd(flow, cmd); >>> >>> MLX5_SET(tls_cmd, cmd, swid, ntohl(handle)); >>> @@ -238,6 +244,8 @@ int mlx5_fpga_tls_resync_rx(struct >>> mlx5_core_dev *mdev, u32 handle, u32 seq, >>> buf->complete = mlx_tls_kfree_complete; >>> >>> ret = mlx5_fpga_sbu_conn_sendmsg(mdev->fpga->tls->conn, buf); >>> + if (ret < 0) >>> + kfree(buf); >>> >>> return ret; >>> } >>>
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c index 5cf5f2a9d51f..8de64e88c670 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c @@ -217,15 +217,21 @@ int mlx5_fpga_tls_resync_rx(struct mlx5_core_dev *mdev, u32 handle, u32 seq, void *cmd; int ret; + rcu_read_lock(); + flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle)); + rcu_read_unlock(); + + if (!flow) { + WARN_ONCE(1, "Received NULL pointer for handle\n"); + return -EINVAL; + } + buf = kzalloc(size, GFP_ATOMIC); if (!buf) return -ENOMEM; cmd = (buf + 1); - rcu_read_lock(); - flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle)); - rcu_read_unlock(); mlx5_fpga_tls_flow_to_cmd(flow, cmd); MLX5_SET(tls_cmd, cmd, swid, ntohl(handle)); @@ -238,6 +244,8 @@ int mlx5_fpga_tls_resync_rx(struct mlx5_core_dev *mdev, u32 handle, u32 seq, buf->complete = mlx_tls_kfree_complete; ret = mlx5_fpga_sbu_conn_sendmsg(mdev->fpga->tls->conn, buf); + if (ret < 0) + kfree(buf); return ret; }
idr_find() can return a NULL value to 'flow' which is used without a check. The patch adds a check to avoid potential NULL pointer dereference. In case of mlx5_fpga_sbu_conn_sendmsg() failure, free buf allocated using kzalloc. Fixes: ab412e1dd7db ("net/mlx5: Accel, add TLS rx offload routines") --- v3: Reorder buf allocations and flow check. v2: failure to return in case of flow failure. v1: Failed to free buf in case of flow failure. Signed-off-by: Aditya Pakki <pakki001@umn.edu> --- drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)