netdev - Re: [PATCH net] net: dsa: validate source trunk against lags

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: <20250731113751.7s7u4zjt6isjnlng@skbuf>
Date: Thu, 31 Jul 2025 14:37:51 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Luke Howard <lukeh@...l.com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Ryan Wilkins <Ryan.Wilkins@...osalliance.com>
Subject: Re: [PATCH net] net: dsa: validate source trunk against lags_len

On Thu, Jul 31, 2025 at 07:55:06PM +1000, Luke Howard wrote:
> Hi Vladimir,
> 
> Thanks for helping me walk through my first kernel patch (I thought I would start with a one line one!).
> 
> > 1. You need to add a Fixes: tag, like the following:
> > Fixes: 5b60dadb71db ("net: dsa: tag_dsa: Support reception of packets from LAG devices")
> 
> Noted.

Further clarification: add the Fixes: tag if you target the patch to the
'net' tree (based on which it will also be backported to stable kernels)
or if the bug was introduced during this kernel development cycle and is
only in 'net-next'. A patch with a 'Fixes' tag for an old commit is
incompatible with submitting it via the 'net-next' tree.

In my previous reply I was only presenting all possibilities; it's still
not clear to me whether this patch should go through net or net-next,
because I haven't understood the circumstances that lead to it yet.

> > 2. The problem statement must not remain in the theoretical realm if you
> >   submit a patch intended as a bug fix. Normally the tagger is used to
> >   process data coming from the switch hardware, so to trigger an
> >   out-of-bounds array access would imply that the problem is elsewhere.
> >   That, or you can make it clear that the patch is to prevent a
> >   modified dsa_loop from crashing when receiving crafted packets over a
> >   regular network interface. But using dsa_loop with a modified
> >   dsa_loop_get_protocol() return value is a developer tool which
> >   involves modifying kernel sources. I would say any fix that doesn't
> >   fix any real life problem in production systems should be sent to
> >   'net-next', not to 'net'. This is in accordance with
> >   Documentation/process/stable-kernel-rules.rst.
> 
> Thanks for the clarification, I was unsure which to send to: I’ll send the revised patch to net-next instead.
> 
> Ryan (on cc) saw this crash with a Marvell switch, using some not yet submitted patches to support in-band switch management without MDIO.
> 
> Exactly what caused the switch to deliver a malformed DSA packet is unknown, but it seems reasonable to expect the kernel to be resilient to this (although one could make an argument that the trust boundary extends to the switch chip).

It does seem reasonable considering dsa_loop, but if we can first
characterize the problem to understand the impact, we should. Then,
this characterization should go into the commit message, to justify to
readers, backporters, etc, when they should worry and when they shouldn't.

If you or Ryan still have access to the buggy system, does the problem
reproduce without the in-band management patches? That is the most
important argument for targetting the 'net' tree. Could you provide an
skb_dump() of its contents? Are you even using offloaded LAG interfaces?

Another rule from Documentation/process/maintainer-netdev.rst is to wait
at least 24 hours between patch revisions.