* [PATCH 2/3] net/mlx5e: Clean up error handling in mlx5e_alloc_flow()
2020-09-27 11:32 [PATCH 0/3] net/mlx5e: Fix some static analysis warnings Alex Dewar
@ 2020-09-27 11:32 ` Alex Dewar
2020-09-28 7:39 ` Leon Romanovsky
2020-09-27 11:32 ` [PATCH 3/3] net/mlx5e: Fix use of freed pointer Alex Dewar
2020-09-27 11:32 ` [PATCH 1/3] net/mlx5e: Fix possible null pointer dereference Alex Dewar
2 siblings, 1 reply; 8+ messages in thread
From: Alex Dewar @ 2020-09-27 11:32 UTC (permalink / raw)
Cc: Alex Dewar, Saeed Mahameed, Leon Romanovsky, David S. Miller,
Jakub Kicinski, netdev, linux-rdma, linux-kernel
The variable flow is used after being allocated but before being
null-checked, which will cause a null pointer dereference if the
allocation failed. Fix this and tidy up the error-checking logic in this
function.
Addresses-Coverity: CID 1497154: Null pointer dereferences (REVERSE_INULL)
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
.../net/ethernet/mellanox/mlx5/core/en_tc.c | 20 ++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index b3c57b984a2a..ed308407be6f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4536,20 +4536,22 @@ mlx5e_alloc_flow(struct mlx5e_priv *priv, int attr_size,
struct mlx5e_tc_flow_parse_attr *parse_attr;
struct mlx5_flow_attr *attr;
struct mlx5e_tc_flow *flow;
- int out_index, err;
+ int out_index;
flow = kzalloc(sizeof(*flow), GFP_KERNEL);
+ if (!flow)
+ return -ENOMEM;
parse_attr = kvzalloc(sizeof(*parse_attr), GFP_KERNEL);
+ if (!parse_attr)
+ goto err_free_flow;
flow->flags = flow_flags;
flow->cookie = f->cookie;
flow->priv = priv;
attr = mlx5_alloc_flow_attr(get_flow_name_space(flow));
- if (!parse_attr || !flow || !attr) {
- err = -ENOMEM;
- goto err_free;
- }
+ if (!attr)
+ goto err_free_parse_attr;
flow->attr = attr;
for (out_index = 0; out_index < MLX5_MAX_FLOW_FWD_VPORTS; out_index++)
@@ -4564,11 +4566,11 @@ mlx5e_alloc_flow(struct mlx5e_priv *priv, int attr_size,
return 0;
-err_free:
- kfree(flow);
+err_free_parse_attr:
kvfree(parse_attr);
- kfree(attr);
- return err;
+err_free_flow:
+ kfree(flow);
+ return -ENOMEM;
}
static void
--
2.28.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/3] net/mlx5e: Clean up error handling in mlx5e_alloc_flow()
2020-09-27 11:32 ` [PATCH 2/3] net/mlx5e: Clean up error handling in mlx5e_alloc_flow() Alex Dewar
@ 2020-09-28 7:39 ` Leon Romanovsky
0 siblings, 0 replies; 8+ messages in thread
From: Leon Romanovsky @ 2020-09-28 7:39 UTC (permalink / raw)
To: Alex Dewar
Cc: Saeed Mahameed, David S. Miller, Jakub Kicinski, netdev,
linux-rdma, linux-kernel
On Sun, Sep 27, 2020 at 12:32:52PM +0100, Alex Dewar wrote:
> The variable flow is used after being allocated but before being
> null-checked, which will cause a null pointer dereference if the
> allocation failed. Fix this and tidy up the error-checking logic in this
> function.
>
> Addresses-Coverity: CID 1497154: Null pointer dereferences (REVERSE_INULL)
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_tc.c | 20 ++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
Thanks, but Gustavo already sent a fix.
https://lore.kernel.org/lkml/20200925164913.GA18472@embeddedor
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] net/mlx5e: Fix use of freed pointer
2020-09-27 11:32 [PATCH 0/3] net/mlx5e: Fix some static analysis warnings Alex Dewar
2020-09-27 11:32 ` [PATCH 2/3] net/mlx5e: Clean up error handling in mlx5e_alloc_flow() Alex Dewar
@ 2020-09-27 11:32 ` Alex Dewar
2020-09-28 7:43 ` Leon Romanovsky
2020-09-27 11:32 ` [PATCH 1/3] net/mlx5e: Fix possible null pointer dereference Alex Dewar
2 siblings, 1 reply; 8+ messages in thread
From: Alex Dewar @ 2020-09-27 11:32 UTC (permalink / raw)
Cc: Alex Dewar, Saeed Mahameed, Leon Romanovsky, David S. Miller,
Jakub Kicinski, Roi Dayan, Oz Shlomo, Paul Blakey,
Ariel Levkovich, Eli Britstein, netdev, linux-rdma, linux-kernel
If the call to mlx5_fc_create() fails, then shared_counter will be freed
before its member, shared_counter->counter, is accessed to retrieve the
error code. Fix by using an intermediate variable.
Addresses-Coverity: CID 1497153: Memory - illegal accesses (USE_AFTER_FREE)
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
index b5f8ed30047b..5851a1dfe6e4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
@@ -738,6 +738,7 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
struct mlx5_ct_shared_counter *shared_counter;
struct mlx5_core_dev *dev = ct_priv->dev;
struct mlx5_ct_entry *rev_entry;
+ struct mlx5_fc *counter;
__be16 tmp_port;
/* get the reversed tuple */
@@ -775,12 +776,13 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
if (!shared_counter)
return ERR_PTR(-ENOMEM);
- shared_counter->counter = mlx5_fc_create(dev, true);
- if (IS_ERR(shared_counter->counter)) {
+ counter = mlx5_fc_create(dev, true);
+ if (IS_ERR(counter)) {
ct_dbg("Failed to create counter for ct entry");
kfree(shared_counter);
- return ERR_PTR(PTR_ERR(shared_counter->counter));
+ return (struct mlx5_ct_shared_counter *)counter;
}
+ shared_counter->counter = counter;
refcount_set(&shared_counter->refcount, 1);
return shared_counter;
--
2.28.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] net/mlx5e: Fix use of freed pointer
2020-09-27 11:32 ` [PATCH 3/3] net/mlx5e: Fix use of freed pointer Alex Dewar
@ 2020-09-28 7:43 ` Leon Romanovsky
2020-09-29 10:15 ` [PATCH v2] " Alex Dewar
0 siblings, 1 reply; 8+ messages in thread
From: Leon Romanovsky @ 2020-09-28 7:43 UTC (permalink / raw)
To: Alex Dewar
Cc: Saeed Mahameed, David S. Miller, Jakub Kicinski, Roi Dayan,
Oz Shlomo, Paul Blakey, Ariel Levkovich, Eli Britstein, netdev,
linux-rdma, linux-kernel
On Sun, Sep 27, 2020 at 12:32:53PM +0100, Alex Dewar wrote:
> If the call to mlx5_fc_create() fails, then shared_counter will be freed
> before its member, shared_counter->counter, is accessed to retrieve the
> error code. Fix by using an intermediate variable.
>
> Addresses-Coverity: CID 1497153: Memory - illegal accesses (USE_AFTER_FREE)
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
Please add Fixes line.
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> index b5f8ed30047b..5851a1dfe6e4 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> @@ -738,6 +738,7 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
> struct mlx5_ct_shared_counter *shared_counter;
> struct mlx5_core_dev *dev = ct_priv->dev;
> struct mlx5_ct_entry *rev_entry;
> + struct mlx5_fc *counter;
> __be16 tmp_port;
>
> /* get the reversed tuple */
> @@ -775,12 +776,13 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
> if (!shared_counter)
> return ERR_PTR(-ENOMEM);
>
> - shared_counter->counter = mlx5_fc_create(dev, true);
> - if (IS_ERR(shared_counter->counter)) {
> + counter = mlx5_fc_create(dev, true);
> + if (IS_ERR(counter)) {
> ct_dbg("Failed to create counter for ct entry");
> kfree(shared_counter);
> - return ERR_PTR(PTR_ERR(shared_counter->counter));
> + return (struct mlx5_ct_shared_counter *)counter;
return ERR_CAST(counter);
> }
> + shared_counter->counter = counter;
>
> refcount_set(&shared_counter->refcount, 1);
> return shared_counter;
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2] net/mlx5e: Fix use of freed pointer
2020-09-28 7:43 ` Leon Romanovsky
@ 2020-09-29 10:15 ` Alex Dewar
2020-09-29 12:37 ` Leon Romanovsky
0 siblings, 1 reply; 8+ messages in thread
From: Alex Dewar @ 2020-09-29 10:15 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Alex Dewar, Saeed Mahameed, David S. Miller, Jakub Kicinski,
Roi Dayan, Oz Shlomo, Paul Blakey, Eli Britstein,
Ariel Levkovich, netdev, linux-rdma, linux-kernel
If the call to mlx5_fc_create() fails, then shared_counter will be freed
before its member, shared_counter->counter, is accessed to retrieve the
error code. Fix by using an intermediate variable.
Addresses-Coverity: CID 1497153: Memory - illegal accesses (USE_AFTER_FREE)
Fixes: 1edae2335adf ("net/mlx5e: CT: Use the same counter for both directions")
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
v2:
- Add Fixes tag (Leon)
- Use ERR_CAST (Leon)
Hi Leon,
I've made the suggested changes. Let me know if there's anything else
you need :)
There is also this patch in the series which doesn't seem to have been
reviewed yet: https://lore.kernel.org/lkml/20200927113254.362480-4-alex.dewar90@gmail.com/
Best,
Alex
drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
index b5f8ed30047b..1e80e7669995 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
@@ -738,6 +738,7 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
struct mlx5_ct_shared_counter *shared_counter;
struct mlx5_core_dev *dev = ct_priv->dev;
struct mlx5_ct_entry *rev_entry;
+ struct mlx5_fc *counter;
__be16 tmp_port;
/* get the reversed tuple */
@@ -775,12 +776,13 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
if (!shared_counter)
return ERR_PTR(-ENOMEM);
- shared_counter->counter = mlx5_fc_create(dev, true);
- if (IS_ERR(shared_counter->counter)) {
+ counter = mlx5_fc_create(dev, true);
+ if (IS_ERR(counter)) {
ct_dbg("Failed to create counter for ct entry");
kfree(shared_counter);
- return ERR_PTR(PTR_ERR(shared_counter->counter));
+ return ERR_CAST(counter);
}
+ shared_counter->counter = counter;
refcount_set(&shared_counter->refcount, 1);
return shared_counter;
--
2.28.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] net/mlx5e: Fix use of freed pointer
2020-09-29 10:15 ` [PATCH v2] " Alex Dewar
@ 2020-09-29 12:37 ` Leon Romanovsky
0 siblings, 0 replies; 8+ messages in thread
From: Leon Romanovsky @ 2020-09-29 12:37 UTC (permalink / raw)
To: Alex Dewar
Cc: Saeed Mahameed, David S. Miller, Jakub Kicinski, Roi Dayan,
Oz Shlomo, Paul Blakey, Eli Britstein, Ariel Levkovich, netdev,
linux-rdma, linux-kernel
On Tue, Sep 29, 2020 at 11:15:49AM +0100, Alex Dewar wrote:
> If the call to mlx5_fc_create() fails, then shared_counter will be freed
> before its member, shared_counter->counter, is accessed to retrieve the
> error code. Fix by using an intermediate variable.
>
> Addresses-Coverity: CID 1497153: Memory - illegal accesses (USE_AFTER_FREE)
> Fixes: 1edae2335adf ("net/mlx5e: CT: Use the same counter for both directions")
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
> v2:
> - Add Fixes tag (Leon)
> - Use ERR_CAST (Leon)
>
> Hi Leon,
>
> I've made the suggested changes. Let me know if there's anything else
> you need :)
Hi Alex,
Saeed already picked Dan's patch.
https://lore.kernel.org/linux-rdma/1017ab3724b83818c03dfa7661b3f31827a7f62f.camel@kernel.org/T/#t
>
> There is also this patch in the series which doesn't seem to have been
> reviewed yet: https://lore.kernel.org/lkml/20200927113254.362480-4-alex.dewar90@gmail.com/
Ariel is handling this internally.
https://lore.kernel.org/linux-rdma/64f6a3eaaac505c341f996df0b0877ee9af56c00.camel@kernel.org/T/#t
Thanks
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] net/mlx5e: Fix possible null pointer dereference
2020-09-27 11:32 [PATCH 0/3] net/mlx5e: Fix some static analysis warnings Alex Dewar
2020-09-27 11:32 ` [PATCH 2/3] net/mlx5e: Clean up error handling in mlx5e_alloc_flow() Alex Dewar
2020-09-27 11:32 ` [PATCH 3/3] net/mlx5e: Fix use of freed pointer Alex Dewar
@ 2020-09-27 11:32 ` Alex Dewar
2 siblings, 0 replies; 8+ messages in thread
From: Alex Dewar @ 2020-09-27 11:32 UTC (permalink / raw)
Cc: Alex Dewar, Saeed Mahameed, Leon Romanovsky, David S. Miller,
Jakub Kicinski, netdev, linux-rdma, linux-kernel
In mlx5e_tc_unoffload_from_slow_path() a null check is performed for the
variable slow_attr and a warning is issued if it is null. However,
slow_attr is used later on in the function regardless. Fix this by
returning if slow_attr is null.
Addresses-Coverity: CID 1497163: Null pointer dereferences (FORWARD_NULL)
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index f815b0c60a6c..b3c57b984a2a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -1238,8 +1238,10 @@ mlx5e_tc_unoffload_from_slow_path(struct mlx5_eswitch *esw,
struct mlx5_flow_attr *slow_attr;
slow_attr = mlx5_alloc_flow_attr(MLX5_FLOW_NAMESPACE_FDB);
- if (!slow_attr)
+ if (!slow_attr) {
mlx5_core_warn(flow->priv->mdev, "Unable to unoffload slow path rule\n");
+ return;
+ }
memcpy(slow_attr, flow->attr, ESW_FLOW_ATTR_SZ);
slow_attr->action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
--
2.28.0
^ permalink raw reply related [flat|nested] 8+ messages in thread