Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 9 May 2023 10:21:48 +0200
From:   Simon Horman <simon.horman@corigine.com>
To:     Dong Chenchen <dongchenchen2@huawei.com>
Cc:     edumazet@google.com, kuba@kernel.org, davem@davemloft.net,
        pabeni@redhat.com, jbenc@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, yuehaibing@huawei.com,
        weiyongjun1@huawei.com
Subject: Re: [PATCH next, v2] net: nsh: Use correct mac_offset to unwind gso
 skb in nsh_gso_segment()
Message-ID: <ZFoCnIIZdrdoht6T@corigine.com>
References: <20230509021924.554576-1-dongchenchen2@huawei.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230509021924.554576-1-dongchenchen2@huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?q32vWeYJgeARxVZrE25433ccC0ozYDm+ea+gXsH3c/iTjQarofqUWp12MThW?=
 =?us-ascii?Q?2c2mj/Jisr/R+I7OQ5ljgqzh5eIhnBrubXr2mXUNVsWOhjAghEt+4dU0bBMr?=
 =?us-ascii?Q?d3xbrSThbEEZdHQtO9tJMHIiXAeT3cykJbjR61e3T+ry+VU1Id62fCfUeyWL?=
 =?us-ascii?Q?co4OzoPeVpIxlbCah8ddm8ZVonMrz0vBtbzaw25/LwhvaRNa0o+D3rwBGHQo?=
 =?us-ascii?Q?CfkKpdE5q2YKlv/yt+2yE8WFrK66lvnL4Ypp+j2ECu8GyoHmFuLkFxDDglXD?=
 =?us-ascii?Q?x2XVLyiom3RFXgdQzkOwaQ/8HP8qWGGIZ+FA+0nw5XGpzfp8pSPUENaXIh6P?=
 =?us-ascii?Q?goXgTG59bBoCTbQDBVzJ/Q+I4QQ5R6L3reqY4oAkVJXivVKrMAffPC1WFf9/?=
 =?us-ascii?Q?8hOplSTiv6giKdKHdk8CEdpbITuyEy1ZI8tCHSmqhuAc0fxRyOp5eyNOOCfF?=
 =?us-ascii?Q?pzkv4vzn1dUtUnxNNYMRBHHQYAylrlYAsl2LJDZC0aqc1Wp2TfOOvRqUcknb?=
 =?us-ascii?Q?gwpS8jPv6sdTEjw1dArhMyJceBiawcRZaNiMyQoXB+ADJYXp3etxsCok3V95?=
 =?us-ascii?Q?JsZGjwfAdyliogM22YSmjUxCogNFRi7Pj7KjqDdZ8v/H1zbTgILSiy9ZyymO?=
 =?us-ascii?Q?v3w7gl+QuXh7xADJF8yS10BVwXSKE61PuYfPN/rP75wDkcDEDOMQemojMOvB?=
 =?us-ascii?Q?XQ4vMsJ0tvvv6HjyLEh6eWuT0v2BXWCLKfPIts2huoCvws5xUw0DXeVg18YI?=
 =?us-ascii?Q?BfMGfN9WGq9Ew2A+hktbTMq1wT07qIQFgz+GTwSajMdJL0GCOlwyjPxqJwyT?=
 =?us-ascii?Q?I4dHWafa4zNB32AoK0NxIWupNr0EN62VMGQApDjNCpIs6msPlOeV2ztSMCP2?=
 =?us-ascii?Q?O0rgqn8ovXQaJgk4MwMG58xLEaaUV9UYp/BCZ56dYOrimEEniqsqgKIFVpFR?=
 =?us-ascii?Q?YbzUw+QeuY2Q4Zr0HzvorkeTAqnlCCjsklSvIUz28k7WLkKqkmEfqJMdkOzK?=
 =?us-ascii?Q?86R14ZWg+pxTDcM2PBwJOrHgnY6kcwVVah8MFEtzyNLS+s8cHvhlyw3LzoYf?=
 =?us-ascii?Q?pOD6dzfcvgXTgOs3NsI9vl9oBMdgwyz2G/J0TDR5S+K5quHB/tIHLFh5qdQG?=
 =?us-ascii?Q?ODeHLRNEGoheHggBUk9ouCjhywGrJoJ5P7WDYtQvGAcdtaPgRmB0lxegHrvI?=
 =?us-ascii?Q?HzyymFZiKpY+HL869BS9bLZKsEBbte4Zj+rf0x4uTsN4+U3DlM4JhS4FR/xt?=
 =?us-ascii?Q?L5zUlTM8VOZQEwXBIMJb8gb3WTku5lDnqFRxXhecLMXil9Bh31+jk4Afw3cw?=
 =?us-ascii?Q?39B8Z62aXRuGN9zVhWwd7gjvoIFGlXbLQ033YxV8EujYHArtSjWKxflJFlJW?=
 =?us-ascii?Q?xfEMsqnvsAR3AySyNQ9Qo0WElZ2ip7vLAgceD4XLiOmFEsjaXwJha7+OTFt7?=
 =?us-ascii?Q?6VSO5W+045n6bOsvKs6Yi4LcFGyQ7ETNzRbIhw/nSDaPKX2ZzTWi8fYj8qMa?=
 =?us-ascii?Q?TwW62OU/mNgUPG+xYA8MpFSN2FcJJ+BGyz9YonkF/QIRgmJoQ1RDtcHOZWmA?=
 =?us-ascii?Q?D4Cp4y1UWEQYWcxUh6yF8LrYPog5h7hL11bjrb/ams9nABIPyBWcn9GrCz5l?=
 =?us-ascii?Q?7KpdmKMy136CuwcPKtCY4TUo7c1OjLnPaZ/s+foc9Xv+lfxwHDFoAUchNgkE?=
 =?us-ascii?Q?iW9cxQ=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f83f1c04-a9fa-4bc5-43e1-08db506673d5
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2023 08:21:56.6051
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: aaCnKzTeJgnYDKx7iGa5EE0grT7mu1RCisjNabIHyg1kgya90A1eUGdfSB9Yx5pO7aOuZtc8GaGnio9xdt5rE2THRnReg22R4FAtrzJa6qA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR13MB5945
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, May 09, 2023 at 10:19:24AM +0800, Dong Chenchen wrote:
> As the call trace shows, skb_panic was caused by wrong skb->mac_header
> in nsh_gso_segment():
> 
> invalid opcode: 0000 [#1] PREEMPT SMP KASAN PTI
> CPU: 3 PID: 2737 Comm: syz Not tainted 6.3.0-next-20230505 #1
> RIP: 0010:skb_panic+0xda/0xe0
> call Trace:
>  skb_push+0x91/0xa0
>  nsh_gso_segment+0x4f3/0x570
>  skb_mac_gso_segment+0x19e/0x270
>  __skb_gso_segment+0x1e8/0x3c0
>  validate_xmit_skb+0x452/0x890
>  validate_xmit_skb_list+0x99/0xd0
>  sch_direct_xmit+0x294/0x7c0
>  __dev_queue_xmit+0x16f0/0x1d70
>  packet_xmit+0x185/0x210
>  packet_snd+0xc15/0x1170
>  packet_sendmsg+0x7b/0xa0
>  sock_sendmsg+0x14f/0x160
> 
> The root cause is:
> nsh_gso_segment() use skb->network_header - nhoff to reset mac_header
> in skb_gso_error_unwind() if inner-layer protocol gso fails.
> However, skb->network_header may be reset by inner-layer protocol
> gso function e.g. mpls_gso_segment. skb->mac_header reset by the
> inaccurate network_header will be larger than skb headroom.
> 
> nsh_gso_segment
>     nhoff = skb->network_header - skb->mac_header;
>     __skb_pull(skb,nsh_len)
>     skb_mac_gso_segment
>         mpls_gso_segment
>             skb_reset_network_header(skb);//skb->network_header+=nsh_len
>             return -EINVAL;
>     skb_gso_error_unwind
>         skb_push(skb, nsh_len);
>         skb->mac_header = skb->network_header - nhoff;
>         // skb->mac_header > skb->headroom, cause skb_push panic
> 
> Use correct mac_offset to restore mac_header to fix it.
> 
> Fixes: c411ed854584 ("nsh: add GSO support")
> Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>

nit: As this is a fix it should probably be targeted at 'net'
     (as opposed to 'net-next'). This should be noted in the subject.

     Subject: [PATCH net v2]...

> ---
> v2:
>   - Use skb->mac_header not skb->network_header-nhoff for mac_offset.
> ---
>  net/nsh/nsh.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/nsh/nsh.c b/net/nsh/nsh.c
> index e9ca007718b7..7eb536a9677f 100644
> --- a/net/nsh/nsh.c
> +++ b/net/nsh/nsh.c
> @@ -78,6 +78,7 @@ static struct sk_buff *nsh_gso_segment(struct sk_buff *skb,
>  {
>  	struct sk_buff *segs = ERR_PTR(-EINVAL);
>  	unsigned int nsh_len, mac_len;
> +	u16 mac_offset = skb->mac_header;

nit: It is generally preferred to arrange local variable in networking code
     from shortest line to longest - reverse xmas tree order.

     This can be verified using.
     https://github.com/ecree-solarflare/xmastree/blob/master/README

>  	__be16 proto;
>  	int nhoff;
>  
> @@ -108,8 +109,7 @@ static struct sk_buff *nsh_gso_segment(struct sk_buff *skb,
>  	segs = skb_mac_gso_segment(skb, features);
>  	if (IS_ERR_OR_NULL(segs)) {
>  		skb_gso_error_unwind(skb, htons(ETH_P_NSH), nsh_len,
> -				     skb->network_header - nhoff,
> -				     mac_len);
> +				     mac_offset, mac_len);
>  		goto out;
>  	}
>  
> -- 
> 2.25.1
> 
>