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.
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 <[email protected]>
---
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);
- 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
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 <[email protected]>
> ---
> 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
>
On 1/11/23 13:19, Simon Horman wrote:
> 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.
>
Yes, it's my typo. I meant 0xFFFFFFFF.
>> 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 <[email protected]>
>> ---
>> 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?
>
As for 'hosts', the expression "2 << (32 - netmask - 1)" is also subject
to overflow, but the type of the variable 'hosts' is u32, and the type
casting gives the correct result. But I will fix it for correctness.
Thank you for review. I will change that in V2.
Ilia.
>> - 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
>>
When first_ip is 0, last_ip is 0xFFFFFFFF, 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.
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 <[email protected]>
---
v2: Fix typo of the last_ip value in the description. Fix the expression for 'hosts'.
net/netfilter/ipset/ip_set_bitmap_ip.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/ipset/ip_set_bitmap_ip.c b/net/netfilter/ipset/ip_set_bitmap_ip.c
index a8ce04a4bb72..e4fa00abde6a 100644
--- a/net/netfilter/ipset/ip_set_bitmap_ip.c
+++ b/net/netfilter/ipset/ip_set_bitmap_ip.c
@@ -308,8 +308,8 @@ bitmap_ip_create(struct net *net, struct ip_set *set, struct nlattr *tb[],
return -IPSET_ERR_BITMAP_RANGE;
pr_debug("mask_bits %u, netmask %u\n", mask_bits, netmask);
- hosts = 2 << (32 - netmask - 1);
- elements = 2 << (netmask - mask_bits - 1);
+ hosts = 2U << (32 - netmask - 1);
+ elements = 2UL << (netmask - mask_bits - 1);
}
if (elements > IPSET_BITMAP_MAX_RANGE + 1)
return -IPSET_ERR_BITMAP_RANGE_SIZE;
--
2.30.2
On Wed, Jan 11, 2023 at 11:57:39AM +0000, Gavrilov Ilia wrote:
> [You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> When first_ip is 0, last_ip is 0xFFFFFFFF, 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.
>
> 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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
> ---
> v2: Fix typo of the last_ip value in the description. Fix the expression for 'hosts'.
> net/netfilter/ipset/ip_set_bitmap_ip.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/ipset/ip_set_bitmap_ip.c b/net/netfilter/ipset/ip_set_bitmap_ip.c
> index a8ce04a4bb72..e4fa00abde6a 100644
> --- a/net/netfilter/ipset/ip_set_bitmap_ip.c
> +++ b/net/netfilter/ipset/ip_set_bitmap_ip.c
> @@ -308,8 +308,8 @@ bitmap_ip_create(struct net *net, struct ip_set *set, struct nlattr *tb[],
> return -IPSET_ERR_BITMAP_RANGE;
>
> pr_debug("mask_bits %u, netmask %u\n", mask_bits, netmask);
> - hosts = 2 << (32 - netmask - 1);
> - elements = 2 << (netmask - mask_bits - 1);
> + hosts = 2U << (32 - netmask - 1);
> + elements = 2UL << (netmask - mask_bits - 1);
> }
> if (elements > IPSET_BITMAP_MAX_RANGE + 1)
> return -IPSET_ERR_BITMAP_RANGE_SIZE;
> --
> 2.30.2
On Wed, Jan 11, 2023 at 01:00:53PM +0100, Simon Horman wrote:
> On Wed, Jan 11, 2023 at 11:57:39AM +0000, Gavrilov Ilia wrote:
> > [You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > When first_ip is 0, last_ip is 0xFFFFFFFF, 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.
> >
> > 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 <[email protected]>
>
> Reviewed-by: Simon Horman <[email protected]>
Applied, thanks