net: ppp: don't call sk_chk_filter twice

From: Christoph Schulz <[email protected]>

Commit 568f194e8bd16c353ad50f9ab95d98b20578a39d ("net: ppp: use sk_unattached_filter api") causes sk_chk_filter() to be called twice when setting a pass or active filter. The first call is from within get_filter(). The second one is through the call chain

ppp_ioctl() --> sk_unattached_filter_create()

              --> __sk_prepare_filter()
              --> sk_chk_filter()

However, sk_chk_filter() is not idempotent as it sometimes replaces filter codes. So running it a second time over the same filter does not work and

yields EINVAL. The net effect is that setting pass and/or active PPP filters does not work anymore, since sk_unattached_filter_create() always returns EINVAL due to the second (and erroneous) call to sk_chk_filter(), regardless whether the filter was sane or not. So this patch simply removes the call to sk_chk_filter() from within get_filter(). This is safe as the filter will be checked by sk_chk_filter() later anyway.

This error is found in exactly the same way in the isdn4linux PPP driver, so it is fixed there the same way.

Signed-off-by Christoph Schulz <[email protected]>

---

drivers/isdn/i4l/isdn_ppp.c +1 -7

index 61ac632..a333b7f 100644
@@ -442,7 +442,7 @@ static int get_filter(void __user *arg, struct sock_filter **p)
 {
        struct sock_fprog uprog;
        struct sock_filter *code = NULL;
-       int len, err;
+       int len;
 
        if (copy_from_user(&uprog, arg, sizeof(uprog)))
                return -EFAULT;

@@ -458,12 +458,6 @@ static int get_filter(void __user *arg, struct sock_filter **p)
        if (IS_ERR(code))
                return PTR_ERR(code);
 
-       err = sk_chk_filter(code, uprog.len);
-       if (err) {
-               kfree(code);
-               return err;
-       }
-
        *p = code;
        return uprog.len;
 }

drivers/net/ppp/ppp_generic.c +1 -8

index 91d6c12..e2f20f8 100644
@@ -539,7 +539,7 @@ static int get_filter(void __user *arg, struct sock_filter **p)
 {
        struct sock_fprog uprog;
        struct sock_filter *code = NULL;
-       int len, err;
+       int len;
 
        if (copy_from_user(&uprog, arg, sizeof(uprog)))
                return -EFAULT;

@@ -554,12 +554,6 @@ static int get_filter(void __user *arg, struct sock_filter **p)
        if (IS_ERR(code))
                return PTR_ERR(code);
 
-       err = sk_chk_filter(code, uprog.len);
-       if (err) {
-               kfree(code);
-               return err;
-       }
-
        *p = code;
        return uprog.len;
 }
--

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 11 lines · Christoph Schulz

From: Christoph Schulz <[email protected]>

Commit 568f194e8bd16c353ad50f9ab95d98b20578a39d ("net: ppp: use sk_unattached_filter api") causes sk_chk_filter() to be called twice when setting a pass or active filter. The first call is from within get_filter(). The second one is through the call chain

   ppp_ioctl() --> sk_unattached_filter_create()
               --> __sk_prepare_filter()
               --> sk_chk_filter()

Quoted 2 lines

However, sk_chk_filter() is not idempotent as it sometimes replaces filter codes. So running it a second time over the same filter does not work and

It's a good thing not to call sk_chk_filter() twice, but the commit log is incorrect.
sk_chk_filter() doesn't replace filter codes anymore.

From: Eric Dumazet <[email protected]>

Add const attribute to filter argument to make clear it is no longer modified.

Signed-off-by Eric Dumazet <[email protected]>

From: Eric Dumazet <[email protected]>

Add const attribute to filter argument to make clear it is no longer modified.

Signed-off-by Eric Dumazet <[email protected]>

---
 include/linux/filter.h |    2 +-
 net/core/filter.c      |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

include/linux/filter.h +1 -1

index b885dcb7eaca..c43c8258e682 100644
@@ -362,7 +362,7 @@ void sk_unattached_filter_destroy(struct sk_filter *fp);
 int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk);
 int sk_detach_filter(struct sock *sk);
 
-int sk_chk_filter(struct sock_filter *filter, unsigned int flen);
+int sk_chk_filter(const struct sock_filter *filter, unsigned int flen);
 int sk_get_filter(struct sock *sk, struct sock_filter __user *filter,
                  unsigned int len);
 

net/core/filter.c +3 -3

index 87af1e3e56c0..b90ae7fb3b89 100644
@@ -1085,7 +1085,7 @@ err:
  * a cell if not previously written, and we check all branches to be sure
  * a malicious user doesn't try to abuse us.
  */
-static int check_load_and_stores(struct sock_filter *filter, int flen)
+static int check_load_and_stores(const struct sock_filter *filter, int flen)
 {
        u16 *masks, memvalid = 0; /* One bit per cell, 16 cells */
        int pc, ret = 0;

@@ -1218,7 +1218,7 @@ static bool chk_code_allowed(u16 code_to_probe)
  *
  * Returns 0 if the rule set is legal or -EINVAL if not.
  */
-int sk_chk_filter(struct sock_filter *filter, unsigned int flen)
+int sk_chk_filter(const struct sock_filter *filter, unsigned int flen)
 {
        bool anc_found;
        int pc;

@@ -1228,7 +1228,7 @@ int sk_chk_filter(struct sock_filter *filter, unsigned int flen)
 
        /* Check the filter code now */
        for (pc = 0; pc < flen; pc++) {
-               struct sock_filter *ftest = &filter[pc];
+               const struct sock_filter *ftest = &filter[pc];
 
                /* May we actually operate on this code? */
                if (!chk_code_allowed(ftest->code))

From: Eric Dumazet <[email protected]>

Add const attribute to filter argument to make clear it is no longer modified.

Signed-off-by Eric Dumazet <[email protected]>

Quoted 2 lines

However, sk_chk_filter() is not idempotent as it sometimes replaces filter codes. So running it a second time over the same filter does not work and

It's a good thing not to call sk_chk_filter() twice, but the commit log is incorrect.
sk_chk_filter() doesn't replace filter codes anymore.

Hello!

Alexei Starovoitov schrieb am Sat, 12 Jul 2014 05:59:46 +0200:

Quoted 8 lines

Quoted 3 lines

However, sk_chk_filter() is not idempotent as it sometimes replaces filter
codes. So running it a second time over the same filter does not work and

It's a good thing not to call sk_chk_filter() twice, but the commit log is incorrect.
sk_chk_filter() doesn't replace filter codes anymore.

Fair enough. Then how should I correctly proceed to submit this patch which fixes a bug in the 3.15 branch (only)? In 3.15.x filter codes _are_ replaced (I just checked the code in 3.15.5). And I originally based my analysis on 3.15.1. Your statement makes the patch an optional improvement for 3.16.x, but it's a necessary fix for 3.15.x. Do I need to submit this patch two times with different commit logs?

I think this patch still makes sense for 'net-next' as cleanup. Just explain it
correctly in the log. It's not needed for 'net'.

As far as stable for 3.15, I'm not yet sure what exactly the problem you're hitting. The way you describe it, the whole ppp filtering shouldn't be working in 3.15...

Also it sounds like you've created a patch out of 3.15 tree, but marked it as 'net-next'. That's not the right. If the tag is 'net-next' it obviously should be based on net-next tree.

Hello!

Alexei Starovoitov schrieb am Sat, 12 Jul 2014 05:59:46 +0200:

Quoted 6 lines

Quoted 2 lines

However, sk_chk_filter() is not idempotent as it sometimes replaces filter codes. So running it a second time over the same filter does not work and

It's a good thing not to call sk_chk_filter() twice, but the commit log is incorrect.
sk_chk_filter() doesn't replace filter codes anymore.

Fair enough. Then how should I correctly proceed to submit this patch which fixes a bug in the 3.15 branch (only)? In 3.15.x filter codes _are_ replaced (I just checked the code in 3.15.5). And I originally based my analysis on 3.15.1. Your statement makes the patch an optional improvement for 3.16.x, but it's a necessary fix for 3.15.x. Do I need to submit this patch two times with different commit logs?

I think the patch makes sense, and you could submit it against net tree (so 'PATCH net' in subject) with a slightly different commit log at the beginning, but the rest could stay explaining that that's the case for 3.15. By that, this could then be picked up into the net tree and thus Dave can queue it for stable inclusion. If you need any help, let me know.