From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
stable@vger.kernel.org, Ben Hawkes <hawkes@google.com>,
Florian Westphal <fw@strlen.de>,
Pablo Neira Ayuso <pablo@netfilter.org>
Subject: [PATCH 3.14 17/29] netfilter: x_tables: fix unconditional helper
Date: Wed, 22 Jun 2016 15:37:25 -0700 [thread overview]
Message-ID: <20160622223531.452514275@linuxfoundation.org> (raw)
In-Reply-To: <20160622223530.496939726@linuxfoundation.org>
3.14-stable review patch. If anyone has any objections, please let me know.
------------------
From: Florian Westphal <fw@strlen.de>
commit 54d83fc74aa9ec72794373cb47432c5f7fb1a309 upstream.
Ben Hawkes says:
In the mark_source_chains function (net/ipv4/netfilter/ip_tables.c) it
is possible for a user-supplied ipt_entry structure to have a large
next_offset field. This field is not bounds checked prior to writing a
counter value at the supplied offset.
Problem is that mark_source_chains should not have been called --
the rule doesn't have a next entry, so its supposed to return
an absolute verdict of either ACCEPT or DROP.
However, the function conditional() doesn't work as the name implies.
It only checks that the rule is using wildcard address matching.
However, an unconditional rule must also not be using any matches
(no -m args).
The underflow validator only checked the addresses, therefore
passing the 'unconditional absolute verdict' test, while
mark_source_chains also tested for presence of matches, and thus
proceeeded to the next (not-existent) rule.
Unify this so that all the callers have same idea of 'unconditional rule'.
Reported-by: Ben Hawkes <hawkes@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
net/ipv4/netfilter/arp_tables.c | 18 +++++++++---------
net/ipv4/netfilter/ip_tables.c | 23 +++++++++++------------
net/ipv6/netfilter/ip6_tables.c | 23 +++++++++++------------
3 files changed, 31 insertions(+), 33 deletions(-)
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -355,11 +355,12 @@ unsigned int arpt_do_table(struct sk_buf
}
/* All zeroes == unconditional rule. */
-static inline bool unconditional(const struct arpt_arp *arp)
+static inline bool unconditional(const struct arpt_entry *e)
{
static const struct arpt_arp uncond;
- return memcmp(arp, &uncond, sizeof(uncond)) == 0;
+ return e->target_offset == sizeof(struct arpt_entry) &&
+ memcmp(&e->arp, &uncond, sizeof(uncond)) == 0;
}
/* Figures out from what hook each rule can be called: returns 0 if
@@ -398,11 +399,10 @@ static int mark_source_chains(const stru
|= ((1 << hook) | (1 << NF_ARP_NUMHOOKS));
/* Unconditional return/END. */
- if ((e->target_offset == sizeof(struct arpt_entry) &&
+ if ((unconditional(e) &&
(strcmp(t->target.u.user.name,
XT_STANDARD_TARGET) == 0) &&
- t->verdict < 0 && unconditional(&e->arp)) ||
- visited) {
+ t->verdict < 0) || visited) {
unsigned int oldpos, size;
if ((strcmp(t->target.u.user.name,
@@ -541,7 +541,7 @@ static bool check_underflow(const struct
const struct xt_entry_target *t;
unsigned int verdict;
- if (!unconditional(&e->arp))
+ if (!unconditional(e))
return false;
t = arpt_get_target_c(e);
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
@@ -588,9 +588,9 @@ static inline int check_entry_size_and_h
newinfo->hook_entry[h] = hook_entries[h];
if ((unsigned char *)e - base == underflows[h]) {
if (!check_underflow(e)) {
- pr_err("Underflows must be unconditional and "
- "use the STANDARD target with "
- "ACCEPT/DROP\n");
+ pr_debug("Underflows must be unconditional and "
+ "use the STANDARD target with "
+ "ACCEPT/DROP\n");
return -EINVAL;
}
newinfo->underflow[h] = underflows[h];
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -168,11 +168,12 @@ get_entry(const void *base, unsigned int
/* All zeroes == unconditional rule. */
/* Mildly perf critical (only if packet tracing is on) */
-static inline bool unconditional(const struct ipt_ip *ip)
+static inline bool unconditional(const struct ipt_entry *e)
{
static const struct ipt_ip uncond;
- return memcmp(ip, &uncond, sizeof(uncond)) == 0;
+ return e->target_offset == sizeof(struct ipt_entry) &&
+ memcmp(&e->ip, &uncond, sizeof(uncond)) == 0;
#undef FWINV
}
@@ -229,11 +230,10 @@ get_chainname_rulenum(const struct ipt_e
} else if (s == e) {
(*rulenum)++;
- if (s->target_offset == sizeof(struct ipt_entry) &&
+ if (unconditional(s) &&
strcmp(t->target.u.kernel.target->name,
XT_STANDARD_TARGET) == 0 &&
- t->verdict < 0 &&
- unconditional(&s->ip)) {
+ t->verdict < 0) {
/* Tail of chains: STANDARD target (return/policy) */
*comment = *chainname == hookname
? comments[NF_IP_TRACE_COMMENT_POLICY]
@@ -472,11 +472,10 @@ mark_source_chains(const struct xt_table
e->comefrom |= ((1 << hook) | (1 << NF_INET_NUMHOOKS));
/* Unconditional return/END. */
- if ((e->target_offset == sizeof(struct ipt_entry) &&
+ if ((unconditional(e) &&
(strcmp(t->target.u.user.name,
XT_STANDARD_TARGET) == 0) &&
- t->verdict < 0 && unconditional(&e->ip)) ||
- visited) {
+ t->verdict < 0) || visited) {
unsigned int oldpos, size;
if ((strcmp(t->target.u.user.name,
@@ -703,7 +702,7 @@ static bool check_underflow(const struct
const struct xt_entry_target *t;
unsigned int verdict;
- if (!unconditional(&e->ip))
+ if (!unconditional(e))
return false;
t = ipt_get_target_c(e);
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
@@ -751,9 +750,9 @@ check_entry_size_and_hooks(struct ipt_en
newinfo->hook_entry[h] = hook_entries[h];
if ((unsigned char *)e - base == underflows[h]) {
if (!check_underflow(e)) {
- pr_err("Underflows must be unconditional and "
- "use the STANDARD target with "
- "ACCEPT/DROP\n");
+ pr_debug("Underflows must be unconditional and "
+ "use the STANDARD target with "
+ "ACCEPT/DROP\n");
return -EINVAL;
}
newinfo->underflow[h] = underflows[h];
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -195,11 +195,12 @@ get_entry(const void *base, unsigned int
/* All zeroes == unconditional rule. */
/* Mildly perf critical (only if packet tracing is on) */
-static inline bool unconditional(const struct ip6t_ip6 *ipv6)
+static inline bool unconditional(const struct ip6t_entry *e)
{
static const struct ip6t_ip6 uncond;
- return memcmp(ipv6, &uncond, sizeof(uncond)) == 0;
+ return e->target_offset == sizeof(struct ip6t_entry) &&
+ memcmp(&e->ipv6, &uncond, sizeof(uncond)) == 0;
}
static inline const struct xt_entry_target *
@@ -255,11 +256,10 @@ get_chainname_rulenum(const struct ip6t_
} else if (s == e) {
(*rulenum)++;
- if (s->target_offset == sizeof(struct ip6t_entry) &&
+ if (unconditional(s) &&
strcmp(t->target.u.kernel.target->name,
XT_STANDARD_TARGET) == 0 &&
- t->verdict < 0 &&
- unconditional(&s->ipv6)) {
+ t->verdict < 0) {
/* Tail of chains: STANDARD target (return/policy) */
*comment = *chainname == hookname
? comments[NF_IP6_TRACE_COMMENT_POLICY]
@@ -482,11 +482,10 @@ mark_source_chains(const struct xt_table
e->comefrom |= ((1 << hook) | (1 << NF_INET_NUMHOOKS));
/* Unconditional return/END. */
- if ((e->target_offset == sizeof(struct ip6t_entry) &&
+ if ((unconditional(e) &&
(strcmp(t->target.u.user.name,
XT_STANDARD_TARGET) == 0) &&
- t->verdict < 0 &&
- unconditional(&e->ipv6)) || visited) {
+ t->verdict < 0) || visited) {
unsigned int oldpos, size;
if ((strcmp(t->target.u.user.name,
@@ -714,7 +713,7 @@ static bool check_underflow(const struct
const struct xt_entry_target *t;
unsigned int verdict;
- if (!unconditional(&e->ipv6))
+ if (!unconditional(e))
return false;
t = ip6t_get_target_c(e);
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
@@ -762,9 +761,9 @@ check_entry_size_and_hooks(struct ip6t_e
newinfo->hook_entry[h] = hook_entries[h];
if ((unsigned char *)e - base == underflows[h]) {
if (!check_underflow(e)) {
- pr_err("Underflows must be unconditional and "
- "use the STANDARD target with "
- "ACCEPT/DROP\n");
+ pr_debug("Underflows must be unconditional and "
+ "use the STANDARD target with "
+ "ACCEPT/DROP\n");
return -EINVAL;
}
newinfo->underflow[h] = underflows[h];
next prev parent reply other threads:[~2016-06-22 22:37 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-22 22:37 [PATCH 3.14 00/29] 3.14.73-stable review Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 01/29] netlink: Fix dump skb leak/double free Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 02/29] sfc: on MC reset, clear PIO buffer linkage in TXQs Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 03/29] tcp: record TLP and ER timer stats in v6 stats Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 05/29] ARM: fix PTRACE_SETVFPREGS on SMP systems Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 06/29] crypto: ccp - Fix AES XTS error for request sizes above 4096 Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 07/29] powerpc: Fix definition of SIAR and SDAR registers Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 08/29] powerpc: Use privileged SPR number for MMCR2 Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 09/29] parisc: Fix pagefault crash in unaligned __get_user() call Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 10/29] ecryptfs: forbid opening files without mmap handler Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 11/29] wext: Fix 32 bit iwpriv compatibility issue with 64 bit Kernel Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 12/29] fix d_walk()/non-delayed __d_free() race Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 13/29] MIPS: Fix 64k page support for 32 bit kernels Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 14/29] powerpc/pseries/eeh: Handle RTAS delay requests in configure_bridge Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 15/29] netfilter: x_tables: validate e->target_offset early Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 16/29] netfilter: x_tables: make sure e->next_offset covers remaining blob size Greg Kroah-Hartman
2016-06-22 22:37 ` Greg Kroah-Hartman [this message]
2016-06-22 22:37 ` [PATCH 3.14 18/29] xfs: fix up backport error in fs/xfs/xfs_inode.c Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 19/29] pipe: limit the per-user amount of pages allocated in pipes Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 20/29] netfilter: x_tables: dont move to non-existent next rule Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 21/29] netfilter: x_tables: validate targets of jumps Greg Kroah-Hartman
2016-06-23 8:54 ` Florian Westphal
2016-06-23 9:13 ` Florian Westphal
2016-06-24 2:46 ` Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 22/29] netfilter: x_tables: add and use xt_check_entry_offsets Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 23/29] netfilter: x_tables: kill check_entry helper Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 24/29] netfilter: x_tables: assert minimum target size Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 25/29] netfilter: x_tables: add compat version of xt_check_entry_offsets Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 26/29] netfilter: x_tables: check standard target size too Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 27/29] netfilter: x_tables: check for bogus target offset Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 28/29] netfilter: x_tables: validate all offsets and sizes in a rule Greg Kroah-Hartman
2016-06-22 22:37 ` [PATCH 3.14 29/29] netfilter: x_tables: dont reject valid target size on some architectures Greg Kroah-Hartman
2016-06-23 4:54 ` [PATCH 3.14 00/29] 3.14.73-stable review -rc2 Greg Kroah-Hartman
2016-06-23 19:41 ` Guenter Roeck
2016-06-23 21:54 ` Shuah Khan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160622223531.452514275@linuxfoundation.org \
--to=gregkh@linuxfoundation.org \
--cc=fw@strlen.de \
--cc=hawkes@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=pablo@netfilter.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox