On Tue, May 9, 2017 at 12:18 AM, Willem de Bruijn <[email protected]> wrote:
> On Mon, May 8, 2017 at 10:22 PM, Willem de Bruijn
> <[email protected]> wrote:
>> On Mon, May 8, 2017 at 4:22 PM, Florian Westphal <[email protected]> wrote:
>>> Richard Guy Briggs <[email protected]> wrote:
>>>
>>> [ CC'ing Willem ]
>>>
>>>> (Summary of IRC conversation for background...)
>>>> Paul Moore and I hit what appears to be a bug since f25's 4.10.11 and
>>>> upstream's 4.11-rc3 that would fail to locate on deletion an icmp rule
>>>> in iptables. Paul narrowed it down to the icmp option.
>>>> Here's our issue where it came up:
>>>>
>>>> https://github.com/linux-audit/audit-testsuite/pull/43#issuecomment-296831880
>>>>
>>>> The test case is:
>>>> # iptables -t mangle -I INPUT -i lo -p icmp --icmp-type 1 -j MARK
>>>> --set-mark 0xdeadbeef
>>>> # iptables -t mangle -D INPUT -i lo -p icmp --icmp-type 1 -j MARK
>>>> --set-mark 0xdeadbeef
>>>> The error we're getting is "iptables: No chain/target/match by that name."
>>>
>>> This is a iptables (yes, userspace) bug exposed with
>>> f77bc5b23fb1af51fc0faa8a479dea8969eb5079
>>> (iptables: use match, target and data copy_to_user helpers)
>>>
>>> icmp is 4 byte in size, but for some silly(?) reason userspace size
>>> in iptables userland is set as XT_ALIGN(sizeof(ipt_icmp), so userspace
>>> thinks its 8.
>>>
>>> In 4.10, kernel copied the full kernel blob to userspace, and since
>>> its allocated with kz/vzalloc the 4 padding bytes are 0.
>>>
>>> libiptc uses malloc, so in case that contains garbage bytes the
>>> memcmp() used to figure out if we found the correct rule in libiptc
>>> during -D mode returns false because it chokes on the extra padding
>>> after struct ipt_icmp match :-/
>>>
>>> Simples fix is to use calloc/memset to 0 in libiptc, but we can't
>>> go with userspace-only fix ...
>>>
>>> So we have to fix this in the kernel and have xt_data_to_user()
>>> zero out any padding as well.
>>>
>>> Willem, if you don't have time to fix this let me know and i'll
>>> try to work on this tomorrow.
>>
>> Thanks, Florian. Also for the detailed context. I'm having a look.
>> The following might be sufficient. It fixes the given example for me.
>>
>> @@ -288,6 +288,10 @@ int xt_data_to_user(void __user *dst, const void *src,
>> usersize = usersize ? : size;
>> if (copy_to_user(dst, src, usersize))
>> return -EFAULT;
>> + size = XT_ALIGN(size);
>> if (usersize != size && clear_user(dst + usersize, size - usersize))
>> return -EFAULT;
>
> This works for me on 64-bit, where __alignof__(struct _xt_align) is 8.
> In a 32-bit environment it is 4. I will need to revise it to work correctly
> for compat. See also COMPAT_XT_ALIGN.
This works for both cases. It requires changing the callers in ebtables,
unfortunately.
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index f134d384852f..29ae5fbd0c08 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -283,12 +283,14 @@ static int xt_obj_to_user(u16 __user *psize, u16 size,
&U->u.user.revision, K->u.kernel.TYPE->revision)
int xt_data_to_user(void __user *dst, const void *src,
- int usersize, int size)
+ int usersize, int size, int aligned_size)
{
usersize = usersize ? : size;
if (copy_to_user(dst, src, usersize))
return -EFAULT;
- if (usersize != size && clear_user(dst + usersize, size - usersize))
+ if (usersize != aligned_size &&
+ clear_user(dst + usersize, aligned_size - usersize))
return -EFAULT;
return 0;
@@ -298,7 +300,9 @@ EXPORT_SYMBOL_GPL(xt_data_to_user);
#define XT_DATA_TO_USER(U, K, TYPE, C_SIZE) \
xt_data_to_user(U->data, K->data, \
K->u.kernel.TYPE->usersize, \
- C_SIZE ? : K->u.kernel.TYPE->TYPE##size)
+ C_SIZE ? : K->u.kernel.TYPE->TYPE##size, \
+ C_SIZE ? COMPAT_XT_ALIGN(C_SIZE) : \
+ XT_ALIGN(K->u.kernel.TYPE->TYPE##size))
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
