[PATCH net-next] net: ppp: don't call sk_chk_filter twice

[PATCH net-next] net: ppp: don't call sk_chk_filter twice

[Christoph Schulz]

From: Christoph Schulz <develop@kristov.de>

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 <develop@kristov.de>
---
diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
index 61ac632..a333b7f 100644
--- a/drivers/isdn/i4l/isdn_ppp.c
+++ b/drivers/isdn/i4l/isdn_ppp.c
@@ -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;
 }
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 91d6c12..e2f20f8 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -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;
 }

Re: [PATCH net-next] net: ppp: don't call sk_chk_filter twice

[Alexei Starovoitov]

On Fri, Jul 11, 2014 at 9:10 AM, Christoph Schulz <develop@kristov.de> wrote:
> From: Christoph Schulz <develop@kristov.de>
>
> 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

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.

> 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 <develop@kristov.de>
> ---
> diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
> index 61ac632..a333b7f 100644
> --- a/drivers/isdn/i4l/isdn_ppp.c
> +++ b/drivers/isdn/i4l/isdn_ppp.c
> @@ -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;
>  }
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 91d6c12..e2f20f8 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] net: ppp: don't call sk_chk_filter twice

[Daniel Borkmann]

On 07/12/2014 05:59 AM, Alexei Starovoitov wrote:
> On Fri, Jul 11, 2014 at 9:10 AM, Christoph Schulz <develop@kristov.de> wrote:
>> From: Christoph Schulz <develop@kristov.de>
>>
>> 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
>
> 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.

Exactly.

[PATCH net-next] net: filter: sk_chk_filter() no longer mangles filter

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

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

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/filter.h |    2 +-
 net/core/filter.c      |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index b885dcb7eaca..c43c8258e682 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -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);
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 87af1e3e56c0..b90ae7fb3b89 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -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))

Re: [PATCH net-next] net: filter: sk_chk_filter() no longer mangles filter

[Daniel Borkmann]

On 07/12/2014 03:49 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Add const attribute to filter argument to make clear it is no
> longer modified.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Good point, thanks Eric.

Acked-by: Daniel Borkmann <dborkman@redhat.com>

Re: [PATCH net-next] net: ppp: don't call sk_chk_filter twice

[Christoph Schulz]

Hello!

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

>> 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?


Thank you in advance,

Christoph Schulz

Re: [PATCH net-next] net: filter: sk_chk_filter() no longer mangles filter

[Alexei Starovoitov]

On Sat, Jul 12, 2014 at 3:49 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Add const attribute to filter argument to make clear it is no
> longer modified.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

makes sense. thanks!

Acked-by: Alexei Starovoitov <ast@plumgrid.com>

> ---
>  include/linux/filter.h |    2 +-
>  net/core/filter.c      |    6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index b885dcb7eaca..c43c8258e682 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -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);
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 87af1e3e56c0..b90ae7fb3b89 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -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))
>
>

Re: [PATCH net-next] net: ppp: don't call sk_chk_filter twice

[Alexei Starovoitov]

On Sat, Jul 12, 2014 at 11:11 PM, Christoph Schulz <develop@kristov.de> wrote:
> Hello!
>
> Alexei Starovoitov schrieb am Sat, 12 Jul 2014 05:59:46 +0200:
>
>
>>> 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.

Re: [PATCH net-next] net: ppp: don't call sk_chk_filter twice

[Christoph Schulz]

Hello!

Alexei Starovoitov schrieb am Sun, 13 Jul 2014 03:44:59 +0200:

> 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'.

OK.

> 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...

I'm afraid this is true. The pppd daemon fails to set the filter(s)  
with the message "Couldn't set pass-filter in kernel: Invalid  
argument". (It doesn't try to set the active-filter because it gives  
up after the first error.) For 3.15.x, you have to apply both of my  
patches to make it work. For 'net', presumably only the second patch  
("fix creating PPP pass and active filters") is necessary because as  
you wrote, sk_chk_filter() can be called multiple times there. See  
also my comment in our bug tracker where the creation of the filter is  
compared between 3.14.x and 3.15.x (I'm afraid the description is in  
German):

https://ssl.nettworks.org/bugs/browse/FFL-858?focusedCommentId=17289&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17289

> 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.

Yes, this was my fault. I'm sorry. I will resubmit the patches soon.


Regards,

Christoph Schulz

Re: [PATCH net-next] net: ppp: don't call sk_chk_filter twice

[Daniel Borkmann]

On 07/12/2014 11:11 PM, Christoph Schulz wrote:
> Hello!
>
> Alexei Starovoitov schrieb am Sat, 12 Jul 2014 05:59:46 +0200:
>
>>> 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.

Thanks again,

Daniel

Re: [PATCH net-next] net: ppp: don't call sk_chk_filter twice

[Christoph Schulz]

Hello!

Am 13.07.2014 12:27, schrieb Daniel Borkmann:

> 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.

I have just resubmitted the patch and hope I did it better this time.
Please let me know whether the commit log is correct now.


Thank you,

Christoph Schulz

Re: [PATCH net-next] net: filter: sk_chk_filter() no longer mangles filter

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 12 Jul 2014 15:49:16 +0200

> From: Eric Dumazet <edumazet@google.com>
> 
> Add const attribute to filter argument to make clear it is no
> longer modified.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.