diff mbox series

drivers/net/ppp: copy userspace array safely

Message ID 20231102191914.52957-2-pstanner@redhat.com
State New
Headers
Series drivers/net/ppp: copy userspace array safely |

Commit Message

Philipp Stanner Nov. 2, 2023, 7:19 p.m. UTC 
  In ppp_generic.c memdup_user() is utilized to copy a userspace array.
This is done without an overflow check.

Use the new wrapper memdup_array_user() to copy the array more safely.

Suggested-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/net/ppp/ppp_generic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Al Viro Nov. 2, 2023, 8:09 p.m. UTC | #1 
On Thu, Nov 02, 2023 at 08:19:15PM +0100, Philipp Stanner wrote:
> In ppp_generic.c memdup_user() is utilized to copy a userspace array.
> This is done without an overflow check.
> 
> Use the new wrapper memdup_array_user() to copy the array more safely.

>  	fprog.len = uprog->len;
> -	fprog.filter = memdup_user(uprog->filter,
> -				   uprog->len * sizeof(struct sock_filter));
> +	fprog.filter = memdup_array_user(uprog->filter,
> +					 uprog->len, sizeof(struct sock_filter));

Far be it from me to discourage security theat^Whardening, but

struct sock_fprog {     /* Required for SO_ATTACH_FILTER. */
        unsigned short          len;    /* Number of filter blocks */
	struct sock_filter __user *filter;
};

struct sock_filter {    /* Filter block */
        __u16   code;   /* Actual filter code */
        __u8    jt;     /* Jump true */
        __u8    jf;     /* Jump false */
        __u32   k;      /* Generic multiuse field */
};

so you might want to mention that overflow in question would have to be
in multiplying an untrusted 16bit value by 8...
Philipp Stanner Nov. 2, 2023, 10:02 p.m. UTC | #2 
Hallo Al,

On Thu, 2023-11-02 at 20:09 +0000, Al Viro wrote:
> On Thu, Nov 02, 2023 at 08:19:15PM +0100, Philipp Stanner wrote:
> > In ppp_generic.c memdup_user() is utilized to copy a userspace
> > array.
> > This is done without an overflow check.
> > 
> > Use the new wrapper memdup_array_user() to copy the array more
> > safely.
> 
> >         fprog.len = uprog->len;
> > -       fprog.filter = memdup_user(uprog->filter,
> > -                                  uprog->len * sizeof(struct
> > sock_filter));
> > +       fprog.filter = memdup_array_user(uprog->filter,
> > +                                        uprog->len, sizeof(struct
> > sock_filter));
> 
> Far be it from me to discourage security theat^Whardening, but


a bit about the background here:
(tl;dr: No reason to worry, I am not one of those security fanatics. In
fact, I worked for 12 months with those people with some mixed
experiences ^^')

(btw, note that the commit says 'safety', not 'security')

We introduced those wrappers to string.h hoping they will be useful.
Now that they're merged, I quickly wanted to establish them as the
standard for copying user-arrays, ideally in the current merge window.
Because its convenient, easy to read and, at times, safer.

I just want to help out a bit in the kernel, clean up here and there;
it's not yet the primary task assigned to me by my employer. Thus, I
quickly prepared 13 patches today implementing the wrapper. You'll find
the others on LKML. Getting to:

> 
> struct sock_fprog {     /* Required for SO_ATTACH_FILTER. */
>         unsigned short          len;    /* Number of filter blocks */
>         struct sock_filter __user *filter;
> };
> 
> struct sock_filter {    /* Filter block */
>         __u16   code;   /* Actual filter code */
>         __u8    jt;     /* Jump true */
>         __u8    jf;     /* Jump false */
>         __u32   k;      /* Generic multiuse field */
> };
> 
> so you might want to mention that overflow in question would have to
> be
> in multiplying an untrusted 16bit value by 8...
> 

I indeed did not even look at that.
When it was obvious to me that fearing an overflow is ridiculous, I
actually adjusted the commit-message, see for example here: [1]

I just didn't see it in ppp. Maybe I should have looked more
intensively for all 13 patches. But we'll get there, that's what v2 and
v3 are for :)

P.


[1] https://lore.kernel.org/all/20231102192402.53721-2-pstanner@redhat.com/


PS: Security != Safety
Al Viro Nov. 2, 2023, 10:30 p.m. UTC | #3 
On Thu, Nov 02, 2023 at 11:02:35PM +0100, Philipp Stanner wrote:

> We introduced those wrappers to string.h hoping they will be useful.
> Now that they're merged, I quickly wanted to establish them as the
> standard for copying user-arrays, ideally in the current merge window.
> Because its convenient, easy to read and, at times, safer.

	They also save future readers a git grep to find the sizes, etc.
Again, the only suggestion is that regarding the commit message;
_some_ of those might end up fixing real overflows and you obviously
want to see how far do those need to be backported, etc.  And "in this
case the overflow doesn't actually happen because <reasons>, but
not having to do such analysis is a good thing" is not a bad explanation
why the primitive in question is useful, IMO.  Granted, in cases like
256 * sizeof(u32) that would be pointless, but for the ones that
are less obvious...

> I just didn't see it in ppp. Maybe I should have looked more
> intensively for all 13 patches. But we'll get there, that's what v2 and
> v3 are for :)

In any case you want to check if there are real bugs caught in that.
diff mbox series

Patch

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index a9beacd552cf..0193af2d31c9 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -570,8 +570,8 @@  static struct bpf_prog *get_filter(struct sock_fprog *uprog)
 
 	/* uprog->len is unsigned short, so no overflow here */
 	fprog.len = uprog->len;
-	fprog.filter = memdup_user(uprog->filter,
-				   uprog->len * sizeof(struct sock_filter));
+	fprog.filter = memdup_array_user(uprog->filter,
+					 uprog->len, sizeof(struct sock_filter));
 	if (IS_ERR(fprog.filter))
 		return ERR_CAST(fprog.filter);