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:   Wed, 31 May 2023 12:12:56 +0200
From:   Simon Horman <simon.horman@corigine.com>
To:     Hangyu Hua <hbh25y@gmail.com>
Cc:     jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, simon.horman@netronome.com,
        pieter.jansen-van-vuuren@amd.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: sched: fix possible OOB write in fl_set_geneve_opt()
Message-ID: <ZHcdqPFZODXF/U7m@corigine.com>
References: <20230529043615.4761-1-hbh25y@gmail.com>
 <ZHXf29es/yh3r6jq@corigine.com>
 <e9925aef-fefc-24b9-dea3-bd3bcca01b35@gmail.com>
 <ZHb/nPuTMja3giSP@corigine.com>
 <ebdc1731-3647-8b58-c66c-db5bb09f5bfa@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ebdc1731-3647-8b58-c66c-db5bb09f5bfa@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ePLBqHPuEB/MNV+UTM/Z3LN0PoaBn24BiDN7Clcu73UvZY6HTGO5S1wOls/U?=
 =?us-ascii?Q?dI5bAIJx3dW+80reF7ahuB1FtlZjmvy2K50cT7F/RvbVsbOSTvLAlODHTCCS?=
 =?us-ascii?Q?txQZyq2hr9lTWZoGWtsm6hXD+1ZnfDCZIZeXNmizNrUgX4DbqXPhhIw9u6ZH?=
 =?us-ascii?Q?FXzd0dDZZA/XSRt5IqExfIa5CfIdKyMw8ajUUUUJsmtx0AwKm+QWr5IPA+Vw?=
 =?us-ascii?Q?E3TpOYwsbBbRR0d4vDBL1M71r1IyUOSKmAHg8bK43xOK/aa+/uhWL/dJCjaJ?=
 =?us-ascii?Q?iV4ojvEChPEZZDWOPrAn9LpbGJmxjrAxlsFJ+K6Z7HXcW+3ObKURYrurCJvR?=
 =?us-ascii?Q?Wl+ameAB2juknnMnrzLj9iiF2JjqCCdmoF0jRRPrbMoEeR5z26k2o4kWWRaC?=
 =?us-ascii?Q?gpvjJ1lkSjzamsyPBT/A3gdGfKDCWe/wL/D6l7qSzhJecWLPhOMKCCfh/Raj?=
 =?us-ascii?Q?Fbh46VJCgt5vzFU6TYSOUFSWI9odhw01jElTM94qsi076NeELkwHZ5x/WEHh?=
 =?us-ascii?Q?FqvVv6FoixMliOP58a6AJYTyu7/ixcgsPOs8YsGCIS8LkqTmrwkGdAlsVtu3?=
 =?us-ascii?Q?YY/8mhjs62Ee0gxSQC/jxmfF92nBsHoJy2KL/Dig0rnqCoCny46/Su7rQWUF?=
 =?us-ascii?Q?PSH7SnXW2/2gv7bofA9Mbyly19/tRzf4StxTk5tvzlvMXv3ZS0nade9uYK2l?=
 =?us-ascii?Q?+U0bftQclN/VrB4YdH7mDceJRuIWHKAXEtmaV41Tp1zojsiDzT8q2Nudh7x2?=
 =?us-ascii?Q?a6VKD4XBtZDXWKgjIBLx6n+pexCFkJ5dqy23O/+j84UzO08IU+nM6XxbUFTJ?=
 =?us-ascii?Q?AFezsQ9mkV6r24rxAFu4CrvSyMuB925lttwGHXI0JQF0n1T7y5b6A2B7KPru?=
 =?us-ascii?Q?zJCgCSyLMg37wgh2zIgnlVrC7zqR1W32qopUe6ny26PM3hpOLu1kUFPBgc6a?=
 =?us-ascii?Q?ZjTUAt+wG+hsEMt3SAxvEUHGt6l+s+uCZIAYXnnwX6LomxSyV3W5eBx9G1yU?=
 =?us-ascii?Q?BRZOWgB/W2D9UQBIxZsGNQDL8b3KKK4ZK24H/0HScI2bCOr9aLOVg28Met3x?=
 =?us-ascii?Q?thZsT33fwpz2RIjD175/p39AYamNvBPQvFwX3LmNt8rhEguFUbspYM3C4G11?=
 =?us-ascii?Q?xHr36lDO6IOS94KnwdhwNZZOnnsu4vO6LQzI+QofIhuFPdIK8wN7Ua9L0JTF?=
 =?us-ascii?Q?CxiOitzvj1iNzA7draWvGThyi50+1fg+FjZaTKPgBUvLRfnOp8RBxjdreDAm?=
 =?us-ascii?Q?FGYB3gFcdRjOsZllU0X0QCE1ge3cujUaWyWLxlA7yTgqmjoN1MTXxISbhsnC?=
 =?us-ascii?Q?GMTnrze7UAM36dOOiPt75ZmamUW9ca21UJjir2FiQBl095TtGyeZRAi0dJHX?=
 =?us-ascii?Q?Yj4fW/cYnTbOylhqsTygzvFfWqaUETOzl6JUulyUkk+nEiEbWQtmoKKTOf9U?=
 =?us-ascii?Q?it8RrZroJPSHGvvd66FVjQv7+K9ibvuHxdlnkvb0VERgYdvM5Gp7GrplyOYG?=
 =?us-ascii?Q?1cF3+s15Uf5Juc6FYUf12+4mnbScgU/4sbCNsbE1J8NPSNgK0rzdnkzwau80?=
 =?us-ascii?Q?fDL5Z30uB124oQkgdH6+biIuTRKYCnqp6KaRD3g+8krR9GWrRVwPY2W1Yk+9?=
 =?us-ascii?Q?JpB2V84dFoE1rXVvOZ89hF/BvOGT/+6YJ/Ts+prfHN7IXEjc5JGngy2ITa8f?=
 =?us-ascii?Q?8o+GdQ=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 64e88831-f81b-4d13-6d29-08db61bf9e7f
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2023 10:13:03.0505
 (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: qH0faYOQozIObEYpRGpfuaVes6Fozxo7W9INn+PrPJ1uLh4t8n+9fh8w8duTtDvAmCoO30LGiIx5m+o+QndZS7gIS0xGcK9p81HCrotVo5c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR13MB6008
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 Wed, May 31, 2023 at 06:05:29PM +0800, Hangyu Hua wrote:
> On 31/5/2023 16:04, Simon Horman wrote:
> > On Wed, May 31, 2023 at 01:38:49PM +0800, Hangyu Hua wrote:
> > > On 30/5/2023 19:36, Simon Horman wrote:
> > > > [Updated Pieter's email address, dropped old email address of mine]
> > > > 
> > > > On Mon, May 29, 2023 at 12:36:15PM +0800, Hangyu Hua wrote:
> > > > > If we send two TCA_FLOWER_KEY_ENC_OPTS_GENEVE packets and their total
> > > > > size is 252 bytes(key->enc_opts.len = 252) then
> > > > > key->enc_opts.len = opt->length = data_len / 4 = 0 when the third
> > > > > TCA_FLOWER_KEY_ENC_OPTS_GENEVE packet enters fl_set_geneve_opt. This
> > > > > bypasses the next bounds check and results in an out-of-bounds.
> > > > > 
> > > > > Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options")
> > > > > Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> > > > 
> > > > Hi Hangyu Hua,
> > > > 
> > > > Thanks. I think I see the problem too.
> > > > But I do wonder, is this more general than Geneve options?
> > > > That is, can this occur with any sequence of options, that
> > > > consume space in enc_opts (configured in fl_set_key()) that
> > > > in total are more than 256 bytes?
> > > > 
> > > 
> > > I think you are right. It is a good idea to add check in fl_set_vxlan_opt
> > > and fl_set_erspan_opt and fl_set_gtp_opt too.
> > > But they should be submitted as other patches. fl_set_geneve_opt has already
> > > check this with the following code:
> > > 
> > > static int fl_set_geneve_opt(const struct nlattr *nla, struct fl_flow_key
> > > *key,
> > > 			     int depth, int option_len,
> > > 			     struct netlink_ext_ack *extack)
> > > {
> > > ...
> > > 		if (new_len > FLOW_DIS_TUN_OPTS_MAX) {
> > > 			NL_SET_ERR_MSG(extack, "Tunnel options exceeds max size");
> > > 			return -ERANGE;
> > > 		}
> > > ...
> > > }
> > > 
> > > This bug will only be triggered under this special
> > > condition(key->enc_opts.len = 252). So I think it will be better understood
> > > by submitting this patch independently.
> > 
> > A considered approach sounds good to me.
> > 
> > I do wonder, could the bounds checks be centralised in the caller?
> > Maybe not if it doesn't know the length that will be consumed.
> > 
> 
> This may make code more complex. I am not sure if it is necessary to do
> this.

Understood. I agree that complex seems undesirable.