| Message ID | 20151023084609.585AEA0C18@unicorn.suse.cz |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello Michal, 2015-10-23, 10:46:09 +0200, Michal Kubecek wrote: > Mainline commit 89c22d8c3b27 ("net: Fix skb csum races when peeking") > backport into pre-3.19 stable kernels introduces a regression causing > null pointer dererefence in skb_copy_and_csum_datagram_iovec(). > > This commit only sets CHECKSUM_UNNECESSARY for non-shared skb, allowing > udp_recvmsg() to take the "else" branch of if (skb_csum_unnecessary(skb)) > when called with null iovec (and len=0, e.g. when peeking for datagram > size first). The problem is that unlike skb_copy_and_csum_datagram_msg() > called in this path since 3.19, skb_copy_and_csum_datagram_iovec() does > not handle null iov parameter and always dereferences iov->iov_len. This > is especially harmful when udp_recvmsg() is called in kernel context, > e.g. from kernel nfsd. > > Band-aid skb_copy_and_csum_datagram_iovec() by testing iov for null and > only checking the checksum in this case. > > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > --- I ran into this problem too and that was my initial solution to this problem as well, but actually, we need a more complete fix, like the one I submitted a few days ago: http://patchwork.ozlabs.org/patch/530642/ With your solution, userspace can still receive bogus EFAULT, or the kernel ends up writing data to an unwanted memory location. Thanks,
On Fri, Oct 23, 2015 at 11:22:19AM +0200, Sabrina Dubroca wrote: > Hello Michal, > > 2015-10-23, 10:46:09 +0200, Michal Kubecek wrote: > > Mainline commit 89c22d8c3b27 ("net: Fix skb csum races when peeking") > > backport into pre-3.19 stable kernels introduces a regression causing > > null pointer dererefence in skb_copy_and_csum_datagram_iovec(). > > > > This commit only sets CHECKSUM_UNNECESSARY for non-shared skb, allowing > > udp_recvmsg() to take the "else" branch of if (skb_csum_unnecessary(skb)) > > when called with null iovec (and len=0, e.g. when peeking for datagram > > size first). The problem is that unlike skb_copy_and_csum_datagram_msg() > > called in this path since 3.19, skb_copy_and_csum_datagram_iovec() does > > not handle null iov parameter and always dereferences iov->iov_len. This > > is especially harmful when udp_recvmsg() is called in kernel context, > > e.g. from kernel nfsd. > > > > Band-aid skb_copy_and_csum_datagram_iovec() by testing iov for null and > > only checking the checksum in this case. > > > > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > > --- > > I ran into this problem too and that was my initial solution to this > problem as well, but actually, we need a more complete fix, like the > one I submitted a few days ago: > > http://patchwork.ozlabs.org/patch/530642/ > > With your solution, userspace can still receive bogus EFAULT, or the > kernel ends up writing data to an unwanted memory location. I must admit I wondered why skb_copy_and_csum_datagram_iovec() doesn't get (and check) read length and why it cannot overfill the buffer. But then I saw the comment "Caller _must_ check that skb will fit to this iovec", stopped thinking and assumed it's OK. I guess I should be less trusting... :-( Thank you for the warning. Michal Kubecek -- 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
On Fri, Oct 23, 2015 at 11:39:46AM +0200, Michal Kubecek wrote: > On Fri, Oct 23, 2015 at 11:22:19AM +0200, Sabrina Dubroca wrote: > > Hello Michal, > > > > 2015-10-23, 10:46:09 +0200, Michal Kubecek wrote: > > > Mainline commit 89c22d8c3b27 ("net: Fix skb csum races when peeking") > > > backport into pre-3.19 stable kernels introduces a regression causing > > > null pointer dererefence in skb_copy_and_csum_datagram_iovec(). > > > > > > This commit only sets CHECKSUM_UNNECESSARY for non-shared skb, allowing > > > udp_recvmsg() to take the "else" branch of if (skb_csum_unnecessary(skb)) > > > when called with null iovec (and len=0, e.g. when peeking for datagram > > > size first). The problem is that unlike skb_copy_and_csum_datagram_msg() > > > called in this path since 3.19, skb_copy_and_csum_datagram_iovec() does > > > not handle null iov parameter and always dereferences iov->iov_len. This > > > is especially harmful when udp_recvmsg() is called in kernel context, > > > e.g. from kernel nfsd. > > > > > > Band-aid skb_copy_and_csum_datagram_iovec() by testing iov for null and > > > only checking the checksum in this case. > > > > > > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > > > --- > > > > I ran into this problem too and that was my initial solution to this > > problem as well, but actually, we need a more complete fix, like the > > one I submitted a few days ago: > > > > http://patchwork.ozlabs.org/patch/530642/ > > > > With your solution, userspace can still receive bogus EFAULT, or the > > kernel ends up writing data to an unwanted memory location. > > I must admit I wondered why skb_copy_and_csum_datagram_iovec() doesn't > get (and check) read length and why it cannot overfill the buffer. But > then I saw the comment "Caller _must_ check that skb will fit to this > iovec", stopped thinking and assumed it's OK. I guess I should be less > trusting... :-( > > Thank you for the warning. > > Michal Kubecek I can confirm that we had this issue reported in our kernels too (https://bugs.launchpad.net/bugs/1508510). I'll queue Sabrina's patch for the next 3.16 stable kernel release. Thanks a lot! Cheers, -- Luís -- 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
diff --git a/net/core/datagram.c b/net/core/datagram.c index 3a402a7b20e9..f8b38794fa9b 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c @@ -799,6 +799,13 @@ int skb_copy_and_csum_datagram_iovec(struct sk_buff *skb, if (!chunk) return 0; + if (!iov) { + if (__skb_checksum_complete(skb)) + goto csum_error; + else + return 0; + } + /* Skip filled elements. * Pretty silly, look at memcpy_toiovec, though 8) */
Mainline commit 89c22d8c3b27 ("net: Fix skb csum races when peeking") backport into pre-3.19 stable kernels introduces a regression causing null pointer dererefence in skb_copy_and_csum_datagram_iovec(). This commit only sets CHECKSUM_UNNECESSARY for non-shared skb, allowing udp_recvmsg() to take the "else" branch of if (skb_csum_unnecessary(skb)) when called with null iovec (and len=0, e.g. when peeking for datagram size first). The problem is that unlike skb_copy_and_csum_datagram_msg() called in this path since 3.19, skb_copy_and_csum_datagram_iovec() does not handle null iov parameter and always dereferences iov->iov_len. This is especially harmful when udp_recvmsg() is called in kernel context, e.g. from kernel nfsd. Band-aid skb_copy_and_csum_datagram_iovec() by testing iov for null and only checking the checksum in this case. Signed-off-by: Michal Kubecek <mkubecek@suse.cz> --- net/core/datagram.c | 7 +++++++ 1 file changed, 7 insertions(+)