| Message ID | 20190318221924.7034-1-pakki001@umn.edu |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: mlx5: Add a missing check on idr_find | expand |
On Mon, Mar 18, 2019 at 05:18:51PM -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. > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > --- > drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c > index 5cf5f2a9d51f..3df468acdffc 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c > @@ -226,6 +226,8 @@ int mlx5_fpga_tls_resync_rx(struct mlx5_core_dev *mdev, u32 handle, u32 seq, > rcu_read_lock(); > flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle)); > rcu_read_unlock(); > + if (!flow) > + return -EINVAL; It is wrong and whole function is wrong too. In such case, you will leak "buf" allocated above. The function mlx5_fpga_sbu_conn_sendmsg() which is used below can fail and it will leave "buf" unfreed too. Thanks > mlx5_fpga_tls_flow_to_cmd(flow, cmd); > > MLX5_SET(tls_cmd, cmd, swid, ntohl(handle)); > -- > 2.17.1 >
On 3/19/2019 12:18 AM, 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. Did you encounter this in practice? This flow you are suggesting shouldn't be possible, because the handle is always there until the socket is destroyed in sk_destruct. But, I wouldn't mind some defensive coding here. Maybe also a WARN_ONCE :) Could you also release buf in case of an error returned from mlx5_fpga_sbu_conn_sendmsg below? Otherwise, I could submit a patch for this.
On Tue, Mar 19, 2019 at 01:41:49PM +0000, Boris Pismenny wrote: > > > On 3/19/2019 12:18 AM, 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. > > Did you encounter this in practice? > This flow you are suggesting shouldn't be possible, because the handle > is always there until the socket is destroyed in sk_destruct. > > But, I wouldn't mind some defensive coding here. > Maybe also a WARN_ONCE :) > > Could you also release buf in case of an error returned from > mlx5_fpga_sbu_conn_sendmsg below? > Otherwise, I could submit a patch for this. Boris, Can you please invest ten seconds to read previous emails prior to answering? https://lkml.org/lkml/2019/3/19/36 Thanks
Hi Leon, On 3/19/2019 4:35 PM, Leon Romanovsky wrote: > On Tue, Mar 19, 2019 at 01:41:49PM +0000, Boris Pismenny wrote: >> >> >> On 3/19/2019 12:18 AM, 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. >> >> Did you encounter this in practice? >> This flow you are suggesting shouldn't be possible, because the handle >> is always there until the socket is destroyed in sk_destruct. >> >> But, I wouldn't mind some defensive coding here. >> Maybe also a WARN_ONCE :) >> >> Could you also release buf in case of an error returned from >> mlx5_fpga_sbu_conn_sendmsg below? >> Otherwise, I could submit a patch for this. > > Boris, > > Can you please invest ten seconds to read previous emails prior to answering? > https://lkml.org/lkml/2019/3/19/36 > The fix you suggested is valid and should be addressed as well. I didn't comment on your reply, but it doesn't mean that I disagree.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c index 5cf5f2a9d51f..3df468acdffc 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c @@ -226,6 +226,8 @@ int mlx5_fpga_tls_resync_rx(struct mlx5_core_dev *mdev, u32 handle, u32 seq, rcu_read_lock(); flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle)); rcu_read_unlock(); + if (!flow) + return -EINVAL; mlx5_fpga_tls_flow_to_cmd(flow, cmd); MLX5_SET(tls_cmd, cmd, swid, ntohl(handle));
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. Signed-off-by: Aditya Pakki <pakki001@umn.edu> --- drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 2 ++ 1 file changed, 2 insertions(+)