diff mbox series

sfc: fix a double-free bug in efx_probe_filters

Message ID 20231214152247.3482788-1-alexious@zju.edu.cn
State New
Headers
Series sfc: fix a double-free bug in efx_probe_filters |

Commit Message

Zhipeng Lu Dec. 14, 2023, 3:22 p.m. UTC 
  In efx_probe_filters, the channel->rps_flow_id is freed in a
efx_for_each_channel marco  when success equals to 0.
However, after the following call chain:

efx_probe_filters
  |-> ef100_net_open
        |-> ef100_net_stop
              |-> efx_remove_filters

The channel->rps_flow_id is freed again in the efx_for_each_channel of
efx_remove_filters, triggering a double-free bug.

Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins")
Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>
---
 drivers/net/ethernet/sfc/rx_common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Simon Horman Dec. 16, 2023, 3:51 p.m. UTC | #1 
On Thu, Dec 14, 2023 at 11:22:46PM +0800, Zhipeng Lu wrote:
> In efx_probe_filters, the channel->rps_flow_id is freed in a
> efx_for_each_channel marco  when success equals to 0.
> However, after the following call chain:
> 
> efx_probe_filters
>   |-> ef100_net_open
>         |-> ef100_net_stop
>               |-> efx_remove_filters

I think the call chain may be a bit more like:

ef100_net_open
  |-> efx_probe_filters
  |-> ef100_net_stop
        |-> efx_remove_filters

> 
> The channel->rps_flow_id is freed again in the efx_for_each_channel of
> efx_remove_filters, triggering a double-free bug.
> 
> Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins")
> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>

The above nit not withstanding, I agree with your reasoning.
And that the problem was introduced in the cited commit.

Reviewed-by: Simon Horman <horms@kernel.org>

...
Paolo Abeni Dec. 19, 2023, 9:19 a.m. UTC | #2 
Hi,

On Thu, 2023-12-14 at 23:22 +0800, Zhipeng Lu wrote:
> In efx_probe_filters, the channel->rps_flow_id is freed in a
> efx_for_each_channel marco  when success equals to 0.
> However, after the following call chain:
> 
> efx_probe_filters
>   |-> ef100_net_open
>         |-> ef100_net_stop
>               |-> efx_remove_filters
> 
> The channel->rps_flow_id is freed again in the efx_for_each_channel of
> efx_remove_filters, triggering a double-free bug.
> 
> Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins")
> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>

The patch LGTM, but could you please update the commit message as per
Simon's suggestions make it more consistent? You can retain Simon's RB
tag.

Thanks!

Paolo
Edward Cree Dec. 20, 2023, 5:09 p.m. UTC | #3 
On 14/12/2023 15:22, Zhipeng Lu wrote:
> In efx_probe_filters, the channel->rps_flow_id is freed in a
> efx_for_each_channel marco  when success equals to 0.
> However, after the following call chain:
> 
> efx_probe_filters
>   |-> ef100_net_open
>         |-> ef100_net_stop
>               |-> efx_remove_filters
> 
> The channel->rps_flow_id is freed again in the efx_for_each_channel of
> efx_remove_filters, triggering a double-free bug.
> 
> Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins")
> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>

Subject line should probably say [PATCH net] to specify the tree.
Modulo that, and Simon's correction to the commit message,

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>
Zhipeng Lu Dec. 20, 2023, 5:30 p.m. UTC | #4 
> On Thu, Dec 14, 2023 at 11:22:46PM +0800, Zhipeng Lu wrote:
> > In efx_probe_filters, the channel->rps_flow_id is freed in a
> > efx_for_each_channel marco  when success equals to 0.
> > However, after the following call chain:
> > 
> > efx_probe_filters
> >   |-> ef100_net_open
> >         |-> ef100_net_stop
> >               |-> efx_remove_filters
> 
> I think the call chain may be a bit more like:
> 
> ef100_net_open
>   |-> efx_probe_filters
>   |-> ef100_net_stop
>         |-> efx_remove_filters
> 
> > 
> > The channel->rps_flow_id is freed again in the efx_for_each_channel of
> > efx_remove_filters, triggering a double-free bug.
> > 
> > Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins")
> > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>
> 
> The above nit not withstanding, I agree with your reasoning.
> And that the problem was introduced in the cited commit.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>

Sorry for the call-chain's problem, I was not familiar with it at that time.
Thanks for Simon's correction. Appreciate!
I'll soon send a v2 patch with the corrected call-chain and RB tags.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
index d2f35ee15eff..fac227d372db 100644
--- a/drivers/net/ethernet/sfc/rx_common.c
+++ b/drivers/net/ethernet/sfc/rx_common.c
@@ -823,8 +823,10 @@  int efx_probe_filters(struct efx_nic *efx)
 		}
 
 		if (!success) {
-			efx_for_each_channel(channel, efx)
+			efx_for_each_channel(channel, efx) {
 				kfree(channel->rps_flow_id);
+				channel->rps_flow_id = NULL;
+			}
 			efx->type->filter_table_remove(efx);
 			rc = -ENOMEM;
 			goto out_unlock;