Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1407392 > unrolled thread
| Started by | John Stultz <john.stultz@linaro.org> |
|---|---|
| First post | 2016-05-26 08:00 +0200 |
| Last post | 2016-05-26 23:10 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[Regression?] iptables broken on 32bit with pre-4.7-rc John Stultz <john.stultz@linaro.org> - 2016-05-26 08:00 +0200
Re: [Regression?] iptables broken on 32bit with pre-4.7-rc Florian Westphal <fw@strlen.de> - 2016-05-26 12:00 +0200
Re: [Regression?] iptables broken on 32bit with pre-4.7-rc John Stultz <john.stultz@linaro.org> - 2016-05-26 23:10 +0200
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2016-05-26 08:00 +0200 |
| Subject | [Regression?] iptables broken on 32bit with pre-4.7-rc |
| Message-ID | <rCWuS-2rd-1@gated-at.bofh.it> |
Hey Florian, Pablo,
In updating a 32bit arm device from 4.6 to Linus' current HEAD, I
noticed I was having some trouble with networking, and realized that
/proc/net/ip_tables_names was suddenly empty.
Digging through the registration process, it seems we're catching on the:
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
target_offset + sizeof(struct xt_standard_target) != next_offset)
return -EINVAL;
check added in 7ed2abddd20cf ("netfilter: x_tables: check standard
target size too").
Where next_offset seems to be 4 bytes larger then the the offset +
standard_target struct size.
Commenting out those checks (the commit doesn't revert cleanly), seems
to get things going again for me.
I'm not exactly sure how the next_offset value is set, so I'm hoping
the proper fix is more obvious to one of you.
thanks
-john
[toc] | [next] | [standalone]
| From | Florian Westphal <fw@strlen.de> |
|---|---|
| Date | 2016-05-26 12:00 +0200 |
| Message-ID | <rD0f8-4Ml-23@gated-at.bofh.it> |
| In reply to | #1407392 |
John Stultz <john.stultz@linaro.org> wrote:
> In updating a 32bit arm device from 4.6 to Linus' current HEAD, I
> noticed I was having some trouble with networking, and realized that
> /proc/net/ip_tables_names was suddenly empty.
>
> Digging through the registration process, it seems we're catching on the:
>
> if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
> target_offset + sizeof(struct xt_standard_target) != next_offset)
> return -EINVAL;
>
> check added in 7ed2abddd20cf ("netfilter: x_tables: check standard
> target size too").
>
> Where next_offset seems to be 4 bytes larger then the the offset +
> standard_target struct size.
I guess its because arm32 needs 8 byte alignment for 64bit
quantities. So we can fix this either via XT_ALIGN()'ing the
target_offset + sizeof() result or by weakening the test to a '>'.
Since we already test proper alignment of start-of-rule in
check_entry_size_and_hooks() I'd suggest we just change the test
to fail only if the next offset is within the min size, i.e.:
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index c69c892..9643047 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -612,7 +612,7 @@ int xt_compat_check_entry_offsets(const void *base, const char *elems,
return -EINVAL;
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
- target_offset + sizeof(struct compat_xt_standard_target) != next_offset)
+ target_offset + sizeof(struct compat_xt_standard_target) > next_offset)
return -EINVAL;
/* compat_xt_entry match has less strict aligment requirements,
@@ -694,7 +694,7 @@ int xt_check_entry_offsets(const void *base,
return -EINVAL;
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
- target_offset + sizeof(struct xt_standard_target) != next_offset)
+ target_offset + sizeof(struct xt_standard_target) > next_offset)
return -EINVAL;
return xt_check_entry_match(elems, base + target_offset,
> I'm not exactly sure how the next_offset value is set, so I'm hoping
> the proper fix is more obvious to one of you.
Its the start of the next rule so it has to be properly aligned
via XT_ALIGN(). Only 32bit system I tested was plain x86 which
only needs 4byte alignment for u64...
Alternative would be something like this:
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index c69c892..ca16c26 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -612,7 +612,7 @@ int xt_compat_check_entry_offsets(const void *base, const char *elems,
return -EINVAL;
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
- target_offset + sizeof(struct compat_xt_standard_target) != next_offset)
+ XT_COMPAT_ALIGN(target_offset + sizeof(struct compat_xt_standard_target)) != next_offset)
return -EINVAL;
/* compat_xt_entry match has less strict aligment requirements,
@@ -694,7 +694,7 @@ int xt_check_entry_offsets(const void *base,
return -EINVAL;
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
- target_offset + sizeof(struct xt_standard_target) != next_offset)
+ XT_ALIGN(target_offset + sizeof(struct xt_standard_target)) != next_offset)
return -EINVAL;
return xt_check_entry_match(elems, base + target_offset,
but afaics the stricter check does not buy anything.
[toc] | [prev] | [next] | [standalone]
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2016-05-26 23:10 +0200 |
| Message-ID | <rDaHw-2Xq-23@gated-at.bofh.it> |
| In reply to | #1407494 |
On Thu, May 26, 2016 at 2:51 AM, Florian Westphal <fw@strlen.de> wrote:
> John Stultz <john.stultz@linaro.org> wrote:
>> In updating a 32bit arm device from 4.6 to Linus' current HEAD, I
>> noticed I was having some trouble with networking, and realized that
>> /proc/net/ip_tables_names was suddenly empty.
>>
>> Digging through the registration process, it seems we're catching on the:
>>
>> if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
>> target_offset + sizeof(struct xt_standard_target) != next_offset)
>> return -EINVAL;
>>
>> check added in 7ed2abddd20cf ("netfilter: x_tables: check standard
>> target size too").
>>
>> Where next_offset seems to be 4 bytes larger then the the offset +
>> standard_target struct size.
>
> I guess its because arm32 needs 8 byte alignment for 64bit
> quantities. So we can fix this either via XT_ALIGN()'ing the
> target_offset + sizeof() result or by weakening the test to a '>'.
>
> Since we already test proper alignment of start-of-rule in
> check_entry_size_and_hooks() I'd suggest we just change the test
> to fail only if the next offset is within the min size, i.e.:
>
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index c69c892..9643047 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -612,7 +612,7 @@ int xt_compat_check_entry_offsets(const void *base, const char *elems,
> return -EINVAL;
>
> if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
> - target_offset + sizeof(struct compat_xt_standard_target) != next_offset)
> + target_offset + sizeof(struct compat_xt_standard_target) > next_offset)
> return -EINVAL;
>
> /* compat_xt_entry match has less strict aligment requirements,
> @@ -694,7 +694,7 @@ int xt_check_entry_offsets(const void *base,
> return -EINVAL;
>
> if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
> - target_offset + sizeof(struct xt_standard_target) != next_offset)
> + target_offset + sizeof(struct xt_standard_target) > next_offset)
> return -EINVAL;
>
> return xt_check_entry_match(elems, base + target_offset,
>
>> I'm not exactly sure how the next_offset value is set, so I'm hoping
>> the proper fix is more obvious to one of you.
>
> Its the start of the next rule so it has to be properly aligned
> via XT_ALIGN(). Only 32bit system I tested was plain x86 which
> only needs 4byte alignment for u64...
>
> Alternative would be something like this:
>
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index c69c892..ca16c26 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -612,7 +612,7 @@ int xt_compat_check_entry_offsets(const void *base, const char *elems,
> return -EINVAL;
>
> if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
> - target_offset + sizeof(struct compat_xt_standard_target) != next_offset)
> + XT_COMPAT_ALIGN(target_offset + sizeof(struct compat_xt_standard_target)) != next_offset)
> return -EINVAL;
>
> /* compat_xt_entry match has less strict aligment requirements,
> @@ -694,7 +694,7 @@ int xt_check_entry_offsets(const void *base,
> return -EINVAL;
>
> if (strcmp(t->u.user.name, XT_STANDARD_TARGET) == 0 &&
> - target_offset + sizeof(struct xt_standard_target) != next_offset)
> + XT_ALIGN(target_offset + sizeof(struct xt_standard_target)) != next_offset)
> return -EINVAL;
>
> return xt_check_entry_match(elems, base + target_offset,
>
>
> but afaics the stricter check does not buy anything.
I can validate that either of these solutions solves the issue for me.
But I'll leave the pick of which to merge up to you. :)
Thanks so much for the quick response!
-john
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web