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, 11 Jan 2023 11:19:47 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru>
Cc:     Pablo Neira Ayuso <pablo@netfilter.org>,
        Jozsef Kadlecsik <kadlec@netfilter.org>,
        Florian Westphal <fw@strlen.de>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "netfilter-devel@vger.kernel.org" <netfilter-devel@vger.kernel.org>,
        "coreteam@netfilter.org" <coreteam@netfilter.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "lvc-project@linuxtesting.org" <lvc-project@linuxtesting.org>
Subject: Re: [PATCH] netfilter: ipset: Fix overflow before widen in the
 bitmap_ip_create() function.
Message-ID: <Y76NQ7tQVB7kE0dG@corigine.com>
References: <20230109115432.3001636-1-Ilia.Gavrilov@infotecs.ru>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230109115432.3001636-1-Ilia.Gavrilov@infotecs.ru>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?4CjDzTURuP6LJ0lxXFjsrTTxKLLYl6UHgM7JoBP0PkN+ck46r5GUmmOnQIHf?=
 =?us-ascii?Q?NVsqvuH2TlAtvgBP+rxpp7g+2hGp80n/4MRQnmhGnXsaqS/bIy/30ckCCB9F?=
 =?us-ascii?Q?FyNBF5Wc7JGaUDHMyIwC1bUImvz8DW2I0Nt+UMsif5crvjwWu1YyyHdUH0rW?=
 =?us-ascii?Q?n6tKeMPCpmUbZgs4a4j/zXJ7rhEfwqnfkgYt0Rs1gO8g+W+or4ofL+IQO7Wl?=
 =?us-ascii?Q?9VmZ5EhxmgLOtN7St5eNv1wNvkZdcF5KKCcpfSCQYNq9J3wUbbXKhXlkkaxE?=
 =?us-ascii?Q?ypsZCat5uRL3W6b5jWy6b9AT0BHSP1Qyz+4mxwQtynFEqxE4i4kDZVOfROMX?=
 =?us-ascii?Q?T/oiQflw+RGY+322HF0nLAoetSb5IY+eO2u5DoImj53GvJOdkBruapQhCvS3?=
 =?us-ascii?Q?iwha2loIslMM+5A1+avG3aK5wU9Px8X9NgJDvcVeq55qrhu/jh7DoXZXXecA?=
 =?us-ascii?Q?a2QGn0Ztnd1deCguO4ioqVH3IwO/OYU4G56M6xW31zl0O0wCGHaT0pcGd3Wv?=
 =?us-ascii?Q?41z+5fENf8y+Q1yBb1BCvv0gk6h09GyW3YBuBVoH9nhp5tuMzBOyN5eTCJ05?=
 =?us-ascii?Q?Z8eyfTQxn7oppSKGbp5ltKUXuqPJXxLPmfm6+IMyurtqOxbvKiuTHm1ORRlw?=
 =?us-ascii?Q?IZbl4Hp03SBwOdj4o49489jYYq4IAYjziNTeT7YuPNJfZWN5jNvPtcpQex4Z?=
 =?us-ascii?Q?hss0rkEpgc0X7xRdrKpVGrEesOCc7ET5LG1P1/gJdclhNKqMppu/E4jJI09E?=
 =?us-ascii?Q?L5T3tQNx9hTYGwfHKluRDozpSF0QJysjw0T8tnEmgCQodt9om3pT7XVqRh8B?=
 =?us-ascii?Q?0NWQq5vNeLqXZATu+U0SaaMqOkcsdq+fCDBtSmTe3rQP4EXGwedT25a1FARh?=
 =?us-ascii?Q?7aBrVBAiutwp3M6g/y5F5QyY9TxYblWbHxtQRciq/6cgjl8P84BbdBPIjIeT?=
 =?us-ascii?Q?bgzV6wkKRrUd8C42KKgxThDBTBfqaXzq7OVybFyXRp52A33cuMkjs0MWiovF?=
 =?us-ascii?Q?YNVekwiM7VuDdQt3/snOqUISmf0UahcpxEq3gioNDnm5EK6vhkpHILWR9lYW?=
 =?us-ascii?Q?VoQDZZmSEBRShjQZakQpsShBMDPVbaCkR14PTbdvYEsrgTRcn8W8kQpSV67f?=
 =?us-ascii?Q?1rL7S7FcSTDf1S9Z8jEzlGuYvb44QXtktFgMNksExrTvcCru8c9bk6g1DH99?=
 =?us-ascii?Q?oTws45UhgS472P/Px494I1NGwIsNf5v6cV9XyukOHbQk9k3jD+7Uguktl+Yx?=
 =?us-ascii?Q?acZoEq6SSb2JhYvSgvSPqidKtpU/nmyeoZbSDaa2PdPRAzL4RFwYJKvjgJI7?=
 =?us-ascii?Q?w5Tz3fi9xGdvYZvo/KvePUMH0E7tF+mrspPFVUMWw82VU+b4BgduAIxYEbC4?=
 =?us-ascii?Q?ayO4sjnbMucX6v3+zKEQ5tXAqYVkAfiw0urPE8AwxiR06PdomtsObAEQt6lx?=
 =?us-ascii?Q?1WU/DB2T0JOSmTU9PHWSQRb6H28srUtoQvW1qEV0M2zxJYNmxj8Evj4vK/lK?=
 =?us-ascii?Q?Y5Nn3s+iaUmzERjdfkhW7Hhq7Nwmk95SZdWdNf2AkWCzkzg8/AUjulR202p1?=
 =?us-ascii?Q?QffSh/D8c213PafxCd4C83TvYMETdUbqOerxLTidJhbLqHalSq1J0EaSdSdT?=
 =?us-ascii?Q?SOvm92N2KlT2Uj79CclYq8KLHmKuhkCkrN0s528Rz69PxIA1SGyAK2pc4izm?=
 =?us-ascii?Q?dVN10g=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5154c18d-8d38-47e1-aca3-08daf3bd6236
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jan 2023 10:19:55.0183
 (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: shcwWUEYf/CxxRFaUy4ex+81NJJfIhe6R/v1EdtpflACVxTydkOo/ei3m76oYEBSC/OnTN0y0SqCHjCp2O7lzA5c9EtKae1rO3NkWaUwop0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR13MB5197
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
        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

Hi Gavrilov,

On Mon, Jan 09, 2023 at 11:54:02AM +0000, Gavrilov Ilia wrote:
> When first_ip is 0, last_ip is 0xFFFFFFF, and netmask is 31, the value of
> an arithmetic expression 2 << (netmask - mask_bits - 1) is subject
> to overflow due to a failure casting operands to a larger data type
> before performing the arithmetic.
> 
> Note that it's harmless since the value will be checked at the next step.

Do you mean 0xFFFFFFFF (8 rather than 8 'F's) ?
If so, I agree with this patch.

> Found by InfoTeCS on behalf of Linux Verification Center
> (linuxtesting.org) with SVACE.
> 
> Fixes: b9fed748185a ("netfilter: ipset: Check and reject crazy /0 input parameters")
> Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>
> ---
>  net/netfilter/ipset/ip_set_bitmap_ip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/ipset/ip_set_bitmap_ip.c b/net/netfilter/ipset/ip_set_bitmap_ip.c
> index a8ce04a4bb72..b8f0fb37378f 100644
> --- a/net/netfilter/ipset/ip_set_bitmap_ip.c
> +++ b/net/netfilter/ipset/ip_set_bitmap_ip.c
> @@ -309,7 +309,7 @@ bitmap_ip_create(struct net *net, struct ip_set *set, struct nlattr *tb[],
>  
>  		pr_debug("mask_bits %u, netmask %u\n", mask_bits, netmask);
>  		hosts = 2 << (32 - netmask - 1);

I think that hosts also overflows, in the case you have described.
Although it also doesn't matter for the same reason you state.
But from a correctness point of view perhaps it should also be addressed?

> -		elements = 2 << (netmask - mask_bits - 1);
> +		elements = 2UL << (netmask - mask_bits - 1);
>  	}
>  	if (elements > IPSET_BITMAP_MAX_RANGE + 1)
>  		return -IPSET_ERR_BITMAP_RANGE_SIZE;
> -- 
> 2.30.2
>