netdev - Re: [PATCH] net/sched: sch_qfq: Fix null-deref in agg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPpSM+Toj3BJyz2NhtSGS2k3io26x+f5eh1cg-_E_6ePARcCmw@mail.gmail.com>
Date: Wed, 2 Jul 2025 00:20:51 -0700
From: Xiang Mei <xmei5@....edu>
To: gregkh@...uxfoundation.org
Cc: netdev@...r.kernel.org, xiyou.wangcong@...il.com, jhs@...atatu.com, 
	jiri@...nulli.us, security@...nel.org
Subject: Re: [PATCH] net/sched: sch_qfq: Fix null-deref in agg_dequeue

I am sorry for the inconvenience and I appreciate your patience and
help. The new patch was sent.


On Wed, Jul 2, 2025 at 12:18 AM Xiang Mei <xmei5@....edu> wrote:
>
> To prevent a potential crash in agg_dequeue (net/sched/sch_qfq.c)
> when cl->qdisc->ops->peek(cl->qdisc) returns NULL, we check the return
> value before using it, similar to the existing approach in sch_hfsc.c.
>
> To avoid code duplication, the following changes are made:
>
> 1. Moved qdisc_warn_nonwc to include/net/sch_generic.h and removed
> its EXPORT_SYMBOL declaration, since all users include the header.
>
> 2. Moved qdisc_peek_len from net/sched/sch_hfsc.c to
> include/net/sch_generic.h so that sch_qfq can reuse it.
>
> 3. Applied qdisc_peek_len in agg_dequeue to avoid crashing.
>
> Signed-off-by: Xiang Mei <xmei5@....edu>
> ---
>  include/net/sch_generic.h | 24 ++++++++++++++++++++++++
>  net/sched/sch_api.c       | 10 ----------
>  net/sched/sch_hfsc.c      | 16 ----------------
>  net/sched/sch_qfq.c       |  2 +-
>  4 files changed, 25 insertions(+), 27 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 3287988a6a98..d090aaa59ef2 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -814,11 +814,35 @@ static inline bool qdisc_tx_is_noop(const struct net_device *dev)
>         return true;
>  }
>
> +static inline void qdisc_warn_nonwc(const char *txt, struct Qdisc *qdisc)
> +{
> +       if (!(qdisc->flags & TCQ_F_WARN_NONWC)) {
> +               pr_warn("%s: %s qdisc %X: is non-work-conserving?\n",
> +                       txt, qdisc->ops->id, qdisc->handle >> 16);
> +               qdisc->flags |= TCQ_F_WARN_NONWC;
> +       }
> +}
> +
>  static inline unsigned int qdisc_pkt_len(const struct sk_buff *skb)
>  {
>         return qdisc_skb_cb(skb)->pkt_len;
>  }
>
> +static inline unsigned int qdisc_peek_len(struct Qdisc *sch)
> +{
> +       struct sk_buff *skb;
> +       unsigned int len;
> +
> +       skb = sch->ops->peek(sch);
> +       if (unlikely(skb == NULL)) {
> +               qdisc_warn_nonwc("qdisc_peek_len", sch);
> +               return 0;
> +       }
> +       len = qdisc_pkt_len(skb);
> +
> +       return len;
> +}
> +
>  /* additional qdisc xmit flags (NET_XMIT_MASK in linux/netdevice.h) */
>  enum net_xmit_qdisc_t {
>         __NET_XMIT_STOLEN = 0x00010000,
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index df89790c459a..6518fdc63dc2 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -594,16 +594,6 @@ void __qdisc_calculate_pkt_len(struct sk_buff *skb,
>         qdisc_skb_cb(skb)->pkt_len = pkt_len;
>  }
>
> -void qdisc_warn_nonwc(const char *txt, struct Qdisc *qdisc)
> -{
> -       if (!(qdisc->flags & TCQ_F_WARN_NONWC)) {
> -               pr_warn("%s: %s qdisc %X: is non-work-conserving?\n",
> -                       txt, qdisc->ops->id, qdisc->handle >> 16);
> -               qdisc->flags |= TCQ_F_WARN_NONWC;
> -       }
> -}
> -EXPORT_SYMBOL(qdisc_warn_nonwc);
> -
>  static enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer)
>  {
>         struct qdisc_watchdog *wd = container_of(timer, struct qdisc_watchdog,
> diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> index afcb83d469ff..751b1e2c35b3 100644
> --- a/net/sched/sch_hfsc.c
> +++ b/net/sched/sch_hfsc.c
> @@ -835,22 +835,6 @@ update_vf(struct hfsc_class *cl, unsigned int len, u64 cur_time)
>         }
>  }
>
> -static unsigned int
> -qdisc_peek_len(struct Qdisc *sch)
> -{
> -       struct sk_buff *skb;
> -       unsigned int len;
> -
> -       skb = sch->ops->peek(sch);
> -       if (unlikely(skb == NULL)) {
> -               qdisc_warn_nonwc("qdisc_peek_len", sch);
> -               return 0;
> -       }
> -       len = qdisc_pkt_len(skb);
> -
> -       return len;
> -}
> -
>  static void
>  hfsc_adjust_levels(struct hfsc_class *cl)
>  {
> diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
> index 5e557b960bde..e0cefa21ce21 100644
> --- a/net/sched/sch_qfq.c
> +++ b/net/sched/sch_qfq.c
> @@ -992,7 +992,7 @@ static struct sk_buff *agg_dequeue(struct qfq_aggregate *agg,
>
>         if (cl->qdisc->q.qlen == 0) /* no more packets, remove from list */
>                 list_del_init(&cl->alist);
> -       else if (cl->deficit < qdisc_pkt_len(cl->qdisc->ops->peek(cl->qdisc))) {
> +       else if (cl->deficit < qdisc_peek_len(cl->qdisc)) {
>                 cl->deficit += agg->lmax;
>                 list_move_tail(&cl->alist, &agg->active);
>         }
> --
> 2.43.0
>