lkml.org 
[lkml]   [2016]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.2 115/115] netfilter: x_tables: fix unconditional helper
    3.2.80-rc1 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: Ben Hutchings <ben@decadent.org.uk>
    ---
    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
    @@ -350,11 +350,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
    @@ -393,11 +394,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,
    @@ -536,7 +536,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)
    @@ -583,9 +583,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
    }

    @@ -230,11 +231,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]
    @@ -468,11 +468,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,
    @@ -699,7 +698,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)
    @@ -747,9 +746,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
    @@ -208,11 +208,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 *
    @@ -269,11 +270,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]
    @@ -490,11 +490,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,
    @@ -722,7 +721,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)
    @@ -770,9 +769,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];
    \
     
     \ /
      Last update: 2016-04-27 03:01    [W:4.085 / U:0.200 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site