[net-next,v2,0/5] net/sched: improve class lifetime handling

Message

Pedro Tammela July 28, 2023, 3:35 p.m. UTC

Valis says[0]:
============
Three classifiers (cls_fw, cls_u32 and cls_route) always copy
tcf_result struct into the new instance of the filter on update.

This causes a problem when updating a filter bound to a class,
as tcf_unbind_filter() is always called on the old instance in the
success path, decreasing filter_cnt of the still referenced class
and allowing it to be deleted, leading to a use-after-free.
============

Turns out these could have been spotted easily with proper warnings.
Improve the current class lifetime with wrappers that check for
overflow/underflow.

While at it add an extack for when a class in use is deleted.

[0] https://lore.kernel.org/all/[e-mail]/

v1 -> v2:
   - Add ack tag from Jamal
   - Move definitions to sch_generic.h as suggested by Cong

Pedro Tammela (5):
  net/sched: wrap open coded Qdics class filter counter
  net/sched: sch_drr: warn about class in use while deleting
  net/sched: sch_hfsc: warn about class in use while deleting
  net/sched: sch_htb: warn about class in use while deleting
  net/sched: sch_qfq: warn about class in use while deleting

 include/net/sch_generic.h | 26 ++++++++++++++++++++++++++
 net/sched/sch_drr.c       | 11 ++++++-----
 net/sched/sch_hfsc.c      | 10 ++++++----
 net/sched/sch_htb.c       | 10 +++++-----
 net/sched/sch_qfq.c       | 12 ++++++------
 5 files changed, 49 insertions(+), 20 deletions(-)

Comments

Simon Horman July 31, 2023, 7:31 a.m. UTC | #1

On Fri, Jul 28, 2023 at 12:35:32PM -0300, Pedro Tammela wrote:
> Valis says[0]:
> ============
> Three classifiers (cls_fw, cls_u32 and cls_route) always copy
> tcf_result struct into the new instance of the filter on update.
> 
> This causes a problem when updating a filter bound to a class,
> as tcf_unbind_filter() is always called on the old instance in the
> success path, decreasing filter_cnt of the still referenced class
> and allowing it to be deleted, leading to a use-after-free.
> ============
> 
> Turns out these could have been spotted easily with proper warnings.
> Improve the current class lifetime with wrappers that check for
> overflow/underflow.
> 
> While at it add an extack for when a class in use is deleted.
> 
> [0] https://lore.kernel.org/all/[e-mail]/
> 
> v1 -> v2:
>    - Add ack tag from Jamal
>    - Move definitions to sch_generic.h as suggested by Cong

Reviewed-by: Simon Horman <[e-mail]>

[e-mail] Aug. 1, 2023, 9:10 a.m. UTC | #2

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <[e-mail]>:

On Fri, 28 Jul 2023 12:35:32 -0300 you wrote:
> Valis says[0]:
> ============
> Three classifiers (cls_fw, cls_u32 and cls_route) always copy
> tcf_result struct into the new instance of the filter on update.
> 
> This causes a problem when updating a filter bound to a class,
> as tcf_unbind_filter() is always called on the old instance in the
> success path, decreasing filter_cnt of the still referenced class
> and allowing it to be deleted, leading to a use-after-free.
> ============
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/5] net/sched: wrap open coded Qdics class filter counter
    https://git.kernel.org/netdev/net-next/c/8798481b667f
  - [net-next,v2,2/5] net/sched: sch_drr: warn about class in use while deleting
    https://git.kernel.org/netdev/net-next/c/daf8d9181b9b
  - [net-next,v2,3/5] net/sched: sch_hfsc: warn about class in use while deleting
    https://git.kernel.org/netdev/net-next/c/8e4553ef3ed5
  - [net-next,v2,4/5] net/sched: sch_htb: warn about class in use while deleting
    https://git.kernel.org/netdev/net-next/c/7118f56e04d4
  - [net-next,v2,5/5] net/sched: sch_qfq: warn about class in use while deleting
    https://git.kernel.org/netdev/net-next/c/e20e75017c5a

You are awesome, thank you!