Re: [PATCH] erspan: Initialize options_len before referencing options.

[Simon Horman <horms@kernel.org>]

On Fri, Dec 12, 2025 at 07:21:49PM +0100, Frode Nordahl wrote:
> On 12/12/25 16:13, Simon Horman wrote:
> > On Fri, Dec 12, 2025 at 07:32:01AM +0000, Frode Nordahl wrote:
> > > The struct ip_tunnel_info has a flexible array member named
> > > options that is protected by a counted_by(options_len)
> > > attribute.
> > > 
> > > The compiler will use this information to enforce runtime bounds
> > > checking deployed by FORTIFY_SOURCE string helpers.
> > > 
> > > As laid out in the GCC documentation, the counter must be
> > > initialized before the first reference to the flexible array
> > > member.
> > > 
> > > In the normal case the ip_tunnel_info_opts_set() helper is used
> > > which would initialize options_len properly, however in the GRE
> > > ERSPAN code a partial update is done, preventing the use of the
> > > helper function.
> > > 
> > > Before this change the handling of ERSPAN traffic in GRE tunnels
> > > would cause a kernel panic when the kernel is compiled with
> > > GCC 15+ and having FORTIFY_SOURCE configured:
> > > 
> > > memcpy: detected buffer overflow: 4 byte write of buffer size 0
> > > 
> > > Call Trace:
> > >   <IRQ>
> > >   __fortify_panic+0xd/0xf
> > >   erspan_rcv.cold+0x68/0x83
> > >   ? ip_route_input_slow+0x816/0x9d0
> > >   gre_rcv+0x1b2/0x1c0
> > >   gre_rcv+0x8e/0x100
> > >   ? raw_v4_input+0x2a0/0x2b0
> > >   ip_protocol_deliver_rcu+0x1ea/0x210
> > >   ip_local_deliver_finish+0x86/0x110
> > >   ip_local_deliver+0x65/0x110
> > >   ? ip_rcv_finish_core+0xd6/0x360
> > >   ip_rcv+0x186/0x1a0
> > > 
> > > Link: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-counted_005fby-variable-attribute
> > > Reported-at: https://launchpad.net/bugs/2129580
> > > Fixes: bb5e62f2d547 ("net: Add options as a flexible array to struct ip_tunnel_info")
> > > Signed-off-by: Frode Nordahl <fnordahl@ubuntu.com>
> > 
> > Hi Frode,
> > 
> > Thanks for your patch (and nice to see you recently in Prague :).
> 
> Thank you for taking the time to review, much appreciated (I enjoyed the
> recent conference in Prague and our exchanges there!).
> 
> > Overall this looks good to me but I have some minor feedback.
> > 
> > 
> > Firstly, the cited patch seems to cover more than erspan.
> > So I'm wondering if you took at look at other cases where
> > this might occur? No problem either way, but if so it might
> > be worth mentioning in the commit message.
> 
> I did some quick searches which formed the basis of the statement of the
> normal case being to use the ip_tunnel_info_opts_set(), I could expand a bit
> upon that statement.

Understood. Whichever way you want to go on this is fine by me.

> > Regarding the comments in the code. I am wondering if the are necessary
> > as the information is also contained in the commit message. And if the
> > source documented every such case then things could get rather verbose.
> > 
> > If you do feel strongly about it keeping it then could I ask that
> > (other than the URL) it is line-wrapped trimmed to 80 columns wide or less,
> > as is still preferred for Networking (but confusingly not all Kernel) code.
> 
> Yes, I guess it became a bit verbose.  The thought was that it would be very
> easy to miss this important detail for anyone (including future me)
> spelunking into this part of the code.
> 
> I'll trim it down to a single line, which should be enough to give the urge
> to look at the commit message.

Thanks.

> > As a fix for code present in net this should be targeted at that tree.
> > It's best to do so explicitly like this:
> > 
> > Subject: [PATCH net] ...
> 
> Ack.
> 
> > And it's probably also best to CC stable@vger.kernel.org.
> > That practice isn't as widespread as perhaps it should be for Networking code.
> > But it does seem worth mentioning.
> 
> Ack, the intention was indeed to Cc them, I only put them into the e-mail
> header and the stable kernel bot pointed out that the Cc also needs to be in
> the commit message.
> 
> -- 
> Frode Nordahl
>