| Message ID | 20231102191914.52957-2-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | drivers/net/ppp: copy userspace array safely | expand |
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...
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
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 --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);
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(-)