netfilter: properly initialize xt_table_info structure

When allocating a xt_table_info structure, we should be clearing out the full amount of memory that was allocated, not just the "header" of the structure. Otherwise odd values could be passed to userspace, which is not a good thing.

Cc stable <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 net/netfilter/x_tables.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/netfilter/x_tables.c +1 -1

index cb7cb300c3bc..a300e8252bb6 100644
@@ -1187,7 +1187,7 @@ struct xt_table_info *xt_alloc_table_info(unsigned int size)
 	if (!info)
 		return NULL;
 
-	memset(info, 0, sizeof(*info));
+	memset(info, 0, sz);
 	info->size = size;
 	return info;
 }

Signature · 1 line

2.17.0

Quoted 19 lines · Greg Kroah-Hartman

When allocating a xt_table_info structure, we should be clearing out the full amount of memory that was allocated, not just the "header" of the structure. Otherwise odd values could be passed to userspace, which is not a good thing.

Cc stable <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 net/netfilter/x_tables.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/netfilter/x_tables.c +0 -0

index cb7cb300c3bc..a300e8252bb6 100644
@@ -1187,7 +1187,7 @@ struct xt_table_info *xt_alloc_table_info(unsigned int size)
 	if (!info)
 		return NULL;
 

Quoted 8 lines

net/netfilter/x_tables.c +1 -1

-	memset(info, 0, sizeof(*info));
+	memset(info, 0, sz);
 	info->size = size;
 	return info;
 }

Signature · 1 line

2.17.0

Or we can replace kvmalloc() by kvzalloc() and remove the memset().

When allocating a xt_table_info structure, we should be clearing out the full amount of memory that was allocated, not just the "header" of the structure. Otherwise odd values could be passed to userspace, which is not a good thing.

Cc stable <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
v2: use kvzalloc instead of kvmalloc/memset pair, as suggested by Michal Kubecek

 net/netfilter/x_tables.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

net/netfilter/x_tables.c +0 -0

index cb7cb300c3bc..cd22bb9b66f3 100644
@@ -1183,11 +1183,10 @@ struct xt_table_info *xt_alloc_table_info(unsigned int size)
 	 * than shoot all processes down before realizing there is nothing
 	 * more to reclaim.

Quoted 26 lines · Greg Kroah-Hartman

When allocating a xt_table_info structure, we should be clearing out the full amount of memory that was allocated, not just the "header" of the structure. Otherwise odd values could be passed to userspace, which is not a good thing.

Cc stable <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
v2: use kvzalloc instead of kvmalloc/memset pair, as suggested by Michal Kubecek

 net/netfilter/x_tables.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

net/netfilter/x_tables.c +1 -1

index cb7cb300c3bc..cd22bb9b66f3 100644
@@ -1183,11 +1183,10 @@ struct xt_table_info *xt_alloc_table_info(unsigned int size)
 	 * than shoot all processes down before realizing there is nothing
 	 * more to reclaim.
 	 */
-	info = kvmalloc(sz, GFP_KERNEL | __GFP_NORETRY);
+	info = kvzalloc(sz, GFP_KERNEL | __GFP_NORETRY);
 	if (!info)
 		return NULL;
 

net/netfilter/x_tables.c struct xt_table_info *xt_alloc_table_info(unsigned int size) · line 1192

Quoted 5 lines

net/netfilter/x_tables.c +0 -1

-	memset(info, 0, sizeof(*info));
 	info->size = size;
 	return info;
 }

I am curious, what particular path does not later overwrite the whole zone ?

Do not get me wrong, this is not fast path, but these blobs can be huge.

Quoted 4 lines

Quoted 4 lines

net/netfilter/x_tables.c +0 -0

@@ -1183,11 +1183,10 @@ struct xt_table_info *xt_alloc_table_info(unsigned int size)
 	 * than shoot all processes down before realizing there is nothing

Quoted 8 lines

net/netfilter/x_tables.c struct xt_table_info *xt_alloc_table_info(unsigned int size) · line 1188

Quoted 6 lines

net/netfilter/x_tables.c +1 -1

 	 * more to reclaim.
 	 */
-	info = kvmalloc(sz, GFP_KERNEL | __GFP_NORETRY);
+	info = kvzalloc(sz, GFP_KERNEL | __GFP_NORETRY);
 	if (!info)
 		return NULL;

I am curious, what particular path does not later overwrite the whole zone ?

In do_ipt_get_ctl, the IPT_SO_GET_ENTRIES: option uses a len value that can be larger than the size of the structure itself.

Then the data is copied to userspace in copy_entries_to_user() for ipv4 and v6, and that's where the "bad data"

Quoted 16 lines · Greg Kroah-Hartman

Quoted 4 lines

Quoted 4 lines

net/netfilter/x_tables.c +0 -0

@@ -1183,11 +1183,10 @@ struct xt_table_info *xt_alloc_table_info(unsigned int size)
 	 * than shoot all processes down before realizing there is nothing

Quoted 8 lines

net/netfilter/x_tables.c struct xt_table_info *xt_alloc_table_info(unsigned int size) · line 1188

Quoted 6 lines

net/netfilter/x_tables.c +1 -1

 	 * more to reclaim.
 	 */
-	info = kvmalloc(sz, GFP_KERNEL | __GFP_NORETRY);
+	info = kvzalloc(sz, GFP_KERNEL | __GFP_NORETRY);
 	if (!info)
 		return NULL;

I am curious, what particular path does not later overwrite the whole zone ?

In do_ipt_get_ctl, the IPT_SO_GET_ENTRIES: option uses a len value that can be larger than the size of the structure itself.

Quoted 2 lines

Then the data is copied to userspace in copy_entries_to_user() for ipv4 and v6, and that's where the "bad data"

If the kernel incorrectly copies more bytes than it should, isn't that a sign that may be going going past the end of the info buffer? (And thus, zeroing won't truly fix the issue)

Quoted 21 lines · Jan Engelhardt

Quoted 18 lines · Greg Kroah-Hartman

Quoted 4 lines

Quoted 4 lines

net/netfilter/x_tables.c +0 -0

@@ -1183,11 +1183,10 @@ struct xt_table_info *xt_alloc_table_info(unsigned int size)
 	 * than shoot all processes down before realizing there is nothing

Quoted 8 lines

net/netfilter/x_tables.c struct xt_table_info *xt_alloc_table_info(unsigned int size) · line 1188

Quoted 6 lines

net/netfilter/x_tables.c +1 -1

 	 * more to reclaim.
 	 */
-	info = kvmalloc(sz, GFP_KERNEL | __GFP_NORETRY);
+	info = kvzalloc(sz, GFP_KERNEL | __GFP_NORETRY);
 	if (!info)
 		return NULL;

I am curious, what particular path does not later overwrite the whole zone ?

In do_ipt_get_ctl, the IPT_SO_GET_ENTRIES: option uses a len value that can be larger than the size of the structure itself.

Then the data is copied to userspace in copy_entries_to_user() for ipv4 and v6, and that's where the "bad data"

Quoted 3 lines

If the kernel incorrectly copies more bytes than it should, isn't that a sign that may be going going past the end of the info buffer? (And thus, zeroing won't truly fix the issue)

No, the buffer size is correct, we just aren't filling up the whole buffer as the data requested is smaller than the buffer size.

Quoted 22 lines · Greg Kroah-Hartman

Quoted 21 lines · Jan Engelhardt

Quoted 18 lines · Greg Kroah-Hartman

Quoted 4 lines

Quoted 4 lines

net/netfilter/x_tables.c +0 -0

@@ -1183,11 +1183,10 @@ struct xt_table_info *xt_alloc_table_info(unsigned int size)
 	 * than shoot all processes down before realizing there is nothing

Quoted 8 lines

net/netfilter/x_tables.c struct xt_table_info *xt_alloc_table_info(unsigned int size) · line 1188

Quoted 6 lines

net/netfilter/x_tables.c +1 -1

 	 * more to reclaim.
 	 */
-	info = kvmalloc(sz, GFP_KERNEL | __GFP_NORETRY);
+	info = kvzalloc(sz, GFP_KERNEL | __GFP_NORETRY);
 	if (!info)
 		return NULL;

I am curious, what particular path does not later overwrite the whole zone ?

In do_ipt_get_ctl, the IPT_SO_GET_ENTRIES: option uses a len value that can be larger than the size of the structure itself.

Then the data is copied to userspace in copy_entries_to_user() for ipv4 and v6, and that's where the "bad data"

Quoted 6 lines

Quoted 3 lines

If the kernel incorrectly copies more bytes than it should, isn't that a sign that may be going going past the end of the info buffer? (And thus, zeroing won't truly fix the issue)

No, the buffer size is correct, we just aren't filling up the whole buffer as the data requested is smaller than the buffer size.

I have no objections to the patch but I'd like to understand what problem its fixing.

Normal pattern is:
newinfo = xt_alloc_table_info(tmp.size); copy_from_user(newinfo->entries, user + sizeof(tmp), tmp.size);

So inital value of the rule blob area should not matter.

Furthermore, when copying the rule blob back to userspace, the kernel is not supposed to copy any padding back to userspace either, since commit f32815d21d4d8287336fb9cef4d2d9e0866214c2 only the user-relevant parts should be copied (some matches and targets allocate kernel-private data such as pointers, and we did use to leak such pointer values back to userspace).

Quoted 22 lines · Greg Kroah-Hartman

Quoted 21 lines · Jan Engelhardt

Quoted 18 lines · Greg Kroah-Hartman

Quoted 4 lines

Quoted 4 lines

net/netfilter/x_tables.c +0 -0

@@ -1183,11 +1183,10 @@ struct xt_table_info *xt_alloc_table_info(unsigned int size)
 	 * than shoot all processes down before realizing there is nothing

Quoted 8 lines

net/netfilter/x_tables.c struct xt_table_info *xt_alloc_table_info(unsigned int size) · line 1188

Quoted 6 lines

net/netfilter/x_tables.c +1 -1

 	 * more to reclaim.
 	 */
-	info = kvmalloc(sz, GFP_KERNEL | __GFP_NORETRY);
+	info = kvzalloc(sz, GFP_KERNEL | __GFP_NORETRY);
 	if (!info)
 		return NULL;

I am curious, what particular path does not later overwrite the whole zone ?

In do_ipt_get_ctl, the IPT_SO_GET_ENTRIES: option uses a len value that can be larger than the size of the structure itself.

Then the data is copied to userspace in copy_entries_to_user() for ipv4 and v6, and that's where the "bad data"

Quoted 6 lines

Quoted 3 lines

If the kernel incorrectly copies more bytes than it should, isn't that a sign that may be going going past the end of the info buffer? (And thus, zeroing won't truly fix the issue)

No, the buffer size is correct, we just aren't filling up the whole buffer as the data requested is smaller than the buffer size.

I have no objections to the patch but I'd like to understand what problem its fixing.

Normal pattern is:
newinfo = xt_alloc_table_info(tmp.size); copy_from_user(newinfo->entries, user + sizeof(tmp), tmp.size);

So inital value of the rule blob area should not matter.

Furthermore, when copying the rule blob back to userspace, the kernel is not supposed to copy any padding back to userspace either, since commit f32815d21d4d8287336fb9cef4d2d9e0866214c2 only the user-relevant parts should be copied (some matches and targets allocate kernel-private data such as pointers, and we did use to leak such pointer values back to userspace).

Hi Greg, I applied this patch on 4.4 and tested it on my Pixel 2, it seems the problem still exists,

Quoted 2 lines · peter pi

Hi Greg, I applied this patch on 4.4 and tested it on my Pixel 2, it seems the problem still exists,

What is the problem exactly?

Quoted 5 lines · Florian Westphal

Quoted 2 lines · peter pi

Hi Greg, I applied this patch on 4.4 and tested it on my Pixel 2, it seems the problem still exists,

What is the problem exactly?

The problem is that kernel data is being sent to userspace due to an uncleared buffer that was allocated and then copied to userspace. This can be reproduced by dumping the current set of iptables rules. Peter had an example reproducing script that he used to specifically show this. Peter, can you provide that?

I thought that initializing this buffer to zero would solve the problem, but I guess I cleared the wrong buffer :(

Quoted 13 lines · Greg Kroah-Hartman

Quoted 5 lines · Florian Westphal

Quoted 2 lines · peter pi

Hi Greg, I applied this patch on 4.4 and tested it on my Pixel 2, it seems the problem still exists,

What is the problem exactly?

The problem is that kernel data is being sent to userspace due to an uncleared buffer that was allocated and then copied to userspace. This can be reproduced by dumping the current set of iptables rules. Peter had an example reproducing script that he used to specifically show this. Peter, can you provide that?

Quoted 2 lines

I thought that initializing this buffer to zero would solve the problem, but I guess I cleared the wrong buffer :(

Never mind, this test was on 4.4 not 4.14.

But even on 4.14 i don't see how zeroing a buffer that will be filled via copy_from_user would help.

Hi Greg,

My test method is very simple:
1, In copy_to_user, add a function call like my_examine(from, n) to check every 8 bytes. There is an kernel function called virt_addr_valid which can check if the value is a address value.
2, Print a kernel log when there is a leak detected in function my_examine 3, Run iptables-save or ip6tables-save in shell, it will hit the kernel code path of the problem

Because my test code is specified for Pixel 2, so I think you can write the test code yourself just about 10 lines code

My test method is very simple:
1, In copy_to_user, add a function call like my_examine(from, n) to check every 8 bytes. There is an kernel function called virt_addr_valid which can check if the value is a address value.
2, Print a kernel log when there is a leak detected in function my_examine 3, Run iptables-save or ip6tables-save in shell, it will hit the kernel code path of the problem

I think I start to understand the problem. IPT_SO_GET_ENTRIES leads to calling copy_entries_to_user() which copies the entries as they are to user provided buffer. It also copies instances of struct xt_entry_match and struct xt_entry_target which contain kernel pointers. We then rewrite them with match/target name for userspace but the layout looks (on x86_64) like this

/* offset | size */ type = struct xt_entry_match {
/* 0 | 32 */ union {
/* 32 */ struct {

/*    0      |     2 */            __u16 match_size;
/*    2      |    29 */            char name[29];
/*   31      |     1 */            __u8 revision;

                                   /* total size (bytes):   32 */
                               } user;

/* 16 */ struct {
/* 0 | 2 */ __u16 match_size;
/* XXX 6-byte hole */
/* 8 | 8 */ struct xt_match *match;

                                   /* total size (bytes):   16 */
                               } kernel;

/* 2 */ __u16 match_size;

                               /* total size (bytes):   32 */
                           } u;

/* 32 | 0 */ unsigned char data[];

                           /* total size (bytes):   32 */
                         }

so that if match name is no longer than five characters (which is often the case), writing to .u.user.name leaves .u.kernel.match untouched. The same problem exists in struct xt_entry_target.

Quoted 29 lines · Michal Kubecek

I think I start to understand the problem. IPT_SO_GET_ENTRIES leads to calling copy_entries_to_user() which copies the entries as they are to user provided buffer. It also copies instances of struct xt_entry_match and struct xt_entry_target which contain kernel pointers. We then rewrite them with match/target name for userspace but the layout looks (on x86_64) like this

/* offset | size */ type = struct xt_entry_match {
/* 0 | 32 */ union {
/* 32 */ struct {

/*    0      |     2 */            __u16 match_size;
/*    2      |    29 */            char name[29];
/*   31      |     1 */            __u8 revision;

                                   /* total size (bytes):   32 */
                               } user;

/* 16 */ struct {
/* 0 | 2 */ __u16 match_size;
/* XXX 6-byte hole */
/* 8 | 8 */ struct xt_match *match;

                                   /* total size (bytes):   16 */
                               } kernel;

/* 2 */ __u16 match_size;

                               /* total size (bytes):   32 */
                           } u;

/* 32 | 0 */ unsigned char data[];

Quoted 7 lines

                           /* total size (bytes):   32 */
                         }

so that if match name is no longer than five characters (which is often the case), writing to .u.user.name leaves .u.kernel.match untouched. The same problem exists in struct xt_entry_target.

And this should no longer happen since the series

f32815d21d4d ("xtables: add xt_match, xt_target and data copy_to_user functions") f77bc5b23fb1 ("iptables: use match, target and data copy_to_user helpers") e47ddb2c4691 ("ip6tables: use match, target and data copy_to_user helpers") 244b531bee2b ("arptables: use match, target and data copy_to_user helpers") b5040f6c33a5 ("ebtables: use match, target and data copy_to_user helpers") 4915f7bbc402 ("xtables: use match, target and data copy_to_user helpers in compat") ec2318904965 ("xtables: extend matches and targets with .usersize")

changed the logic in 4.11-rc1.