* [PATCH] drivers/net/ppp: copy userspace array safely @ 2023-11-02 19:19 Philipp Stanner 2023-11-02 20:09 ` Al Viro 0 siblings, 1 reply; 4+ messages in thread From: Philipp Stanner @ 2023-11-02 19:19 UTC (permalink / raw) To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Stanislav Fomichev, Greg Kroah-Hartman, Benjamin Tissoires, Al Viro Cc: linux-ppp, netdev, linux-kernel, Philipp Stanner, Dave Airlie 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(-) 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); -- 2.41.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drivers/net/ppp: copy userspace array safely 2023-11-02 19:19 [PATCH] drivers/net/ppp: copy userspace array safely Philipp Stanner @ 2023-11-02 20:09 ` Al Viro 2023-11-02 22:02 ` Philipp Stanner 0 siblings, 1 reply; 4+ messages in thread From: Al Viro @ 2023-11-02 20:09 UTC (permalink / raw) To: Philipp Stanner Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Stanislav Fomichev, Greg Kroah-Hartman, Benjamin Tissoires, linux-ppp, netdev, linux-kernel, Dave Airlie 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... ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drivers/net/ppp: copy userspace array safely 2023-11-02 20:09 ` Al Viro @ 2023-11-02 22:02 ` Philipp Stanner 2023-11-02 22:30 ` Al Viro 0 siblings, 1 reply; 4+ messages in thread From: Philipp Stanner @ 2023-11-02 22:02 UTC (permalink / raw) To: Al Viro Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Stanislav Fomichev, Greg Kroah-Hartman, Benjamin Tissoires, linux-ppp, netdev, linux-kernel, Dave Airlie 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drivers/net/ppp: copy userspace array safely 2023-11-02 22:02 ` Philipp Stanner @ 2023-11-02 22:30 ` Al Viro 0 siblings, 0 replies; 4+ messages in thread From: Al Viro @ 2023-11-02 22:30 UTC (permalink / raw) To: Philipp Stanner Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Stanislav Fomichev, Greg Kroah-Hartman, Benjamin Tissoires, linux-ppp, netdev, linux-kernel, Dave Airlie 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-02 22:30 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-02 19:19 [PATCH] drivers/net/ppp: copy userspace array safely Philipp Stanner 2023-11-02 20:09 ` Al Viro 2023-11-02 22:02 ` Philipp Stanner 2023-11-02 22:30 ` Al Viro
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).