* [PATCH] drbd: Fix kernel_sendmsg() usage
@ 2016-11-08 10:43 Richard Weinberger
2016-11-08 13:55 ` Richard Weinberger
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Richard Weinberger @ 2016-11-08 10:43 UTC (permalink / raw)
To: drbd-dev
Cc: linux-kernel, lars.ellenberg, philipp.reisner, Richard Weinberger,
stable, viro, christoph.lechleitner, wolfgang.glas
Don't pass a size larger than iov_len to kernel_sendmsg().
Otherwise it will cause a NULL pointer deref when kernel_sendmsg()
returns with rv < size.
Although the issue exists since day 0, only on non-ancient kernels
that contain change 57be5bdad759 ("ip: convert tcp_sendmsg() to iov_iter
primitives") it seems to trigger [0][1][2][3][4].
[0] http://lists.linbit.com/pipermail/drbd-user/2016-July/023112.html
[1] http://lists.linbit.com/pipermail/drbd-dev/2016-March/003362.html
[2] https://forums.grsecurity.net/viewtopic.php?f=3&t=4546
[3] https://ubuntuforums.org/showthread.php?t=2336150
[4] http://e2.howsolveproblem.com/i/1175162/
Fixes: b411b3637fa71fc ("The DRBD driver")
Cc: stable@vger.kernel.org
Cc: viro@zeniv.linux.org.uk
Cc: christoph.lechleitner@iteg.at
Cc: wolfgang.glas@iteg.at
Reported-by: Christoph Lechleitner <christoph.lechleitner@iteg.at>
Tested-by: Christoph Lechleitner <christoph.lechleitner@iteg.at>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
drivers/block/drbd/drbd_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 100be556e613..cbec781c2b57 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -1871,7 +1871,7 @@ int drbd_send(struct drbd_connection *connection, struct socket *sock,
drbd_update_congested(connection);
}
do {
- rv = kernel_sendmsg(sock, &msg, &iov, 1, size);
+ rv = kernel_sendmsg(sock, &msg, &iov, 1, size - sent);
if (rv == -EAGAIN) {
if (we_should_drop_the_connection(connection, sock))
break;
--
2.7.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] drbd: Fix kernel_sendmsg() usage 2016-11-08 10:43 [PATCH] drbd: Fix kernel_sendmsg() usage Richard Weinberger @ 2016-11-08 13:55 ` Richard Weinberger 2016-11-08 16:52 ` Jens Axboe 2016-11-08 14:03 ` [PATCH] drbd: Fix kernel_sendmsg() usage Christoph Lechleitner 2016-11-08 15:49 ` Christoph Hellwig 2 siblings, 1 reply; 13+ messages in thread From: Richard Weinberger @ 2016-11-08 13:55 UTC (permalink / raw) To: Jens Axboe, linux-kernel, stable, drbd-dev, philipp.reisner, viro, christoph.lechleitner, wolfgang.glas Lars, On 08.11.2016 14:43, Lars Ellenberg wrote: > From 3a5859e696178e31a25e65de58c461046fc52beb Mon Sep 17 00:00:00 2001 > From: Richard Weinberger <richard@nod.at> > Date: Tue, 8 Nov 2016 11:43:09 +0100 > Subject: [PATCH] drbd: Fix kernel_sendmsg() usage - potential NULL deref > drbd: Fix kernel_sendmsg() usage - potential NULL deref > > Don't pass a size larger than iov_len to kernel_sendmsg(). > Otherwise it will cause a NULL pointer deref when kernel_sendmsg() > returns with rv < size. > > DRBD as external module has been around in the kernel 2.4 days already. > We used to be compatible to 2.4 and very early 2.6 kernels, > we used to use > rv = sock_sendmsg(sock, &msg, iov.iov_len); > then later changed to > rv = kernel_sendmsg(sock, &msg, &iov, 1, size); > when we should have used > rv = kernel_sendmsg(sock, &msg, &iov, 1, iov.iov_len); > > tcp_sendmsg() used to totally ignore the size parameter. > 57be5bd ip: convert tcp_sendmsg() to iov_iter primitives > changes that, and exposes our long standing error. > > Even with this error exposed, to trigger the bug, we would need to have > an environment (config or otherwise) causing us to not use sendpage() > for larger transfers, a flaky connection, and have it fail "just at the > right time". Apparently that was unlikely enough for most, so this went > unnoticed for years. > > Still, it is known to trigger at least some of these, > and suspected for the others: > [0] http://lists.linbit.com/pipermail/drbd-user/2016-July/023112.html > [1] http://lists.linbit.com/pipermail/drbd-dev/2016-March/003362.html > [2] https://forums.grsecurity.net/viewtopic.php?f=3&t=4546 > [3] https://ubuntuforums.org/showthread.php?t=2336150 > [4] http://e2.howsolveproblem.com/i/1175162/ > > This should go into 4.9, > and into all stable branches since and including v4.0, > which is the first to contain the exposing change. > > It is correct for all stable branches older than that as well > (which contain the DRBD driver; which is 2.6.33 and up). > > It requires a small "conflict" resolution for v4.4 and earlier, with v4.5 > we dropped the comment block immediately preceding the kernel_sendmsg(). > > Cc: stable@vger.kernel.org > Cc: viro@zeniv.linux.org.uk > Cc: christoph.lechleitner@iteg.at > Cc: wolfgang.glas@iteg.at > Reported-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> > Tested-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> > Signed-off-by: Richard Weinberger <richard@nod.at> > Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com> Changing my patch is perfectly fine, but please clearly state it. I.e. by adding something like that before your S-o-b. [Lars: Massaged patch to match my personal taste...] Thanks, //richard ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drbd: Fix kernel_sendmsg() usage 2016-11-08 13:55 ` Richard Weinberger @ 2016-11-08 16:52 ` Jens Axboe 2016-11-09 15:32 ` Lars Ellenberg 0 siblings, 1 reply; 13+ messages in thread From: Jens Axboe @ 2016-11-08 16:52 UTC (permalink / raw) To: Richard Weinberger Cc: linux-kernel, stable, drbd-dev, philipp.reisner, viro, christoph.lechleitner, wolfgang.glas On Tue, Nov 08 2016, Richard Weinberger wrote: > On 08.11.2016 14:43, Lars Ellenberg wrote: > > From 3a5859e696178e31a25e65de58c461046fc52beb Mon Sep 17 00:00:00 2001 > > From: Richard Weinberger <richard@nod.at> > > Date: Tue, 8 Nov 2016 11:43:09 +0100 > > Subject: [PATCH] drbd: Fix kernel_sendmsg() usage - potential NULL deref > > drbd: Fix kernel_sendmsg() usage - potential NULL deref > > > > Don't pass a size larger than iov_len to kernel_sendmsg(). > > Otherwise it will cause a NULL pointer deref when kernel_sendmsg() > > returns with rv < size. > > > > DRBD as external module has been around in the kernel 2.4 days already. > > We used to be compatible to 2.4 and very early 2.6 kernels, > > we used to use > > rv = sock_sendmsg(sock, &msg, iov.iov_len); > > then later changed to > > rv = kernel_sendmsg(sock, &msg, &iov, 1, size); > > when we should have used > > rv = kernel_sendmsg(sock, &msg, &iov, 1, iov.iov_len); > > > > tcp_sendmsg() used to totally ignore the size parameter. > > 57be5bd ip: convert tcp_sendmsg() to iov_iter primitives > > changes that, and exposes our long standing error. > > > > Even with this error exposed, to trigger the bug, we would need to have > > an environment (config or otherwise) causing us to not use sendpage() > > for larger transfers, a flaky connection, and have it fail "just at the > > right time". Apparently that was unlikely enough for most, so this went > > unnoticed for years. > > > > Still, it is known to trigger at least some of these, > > and suspected for the others: > > [0] http://lists.linbit.com/pipermail/drbd-user/2016-July/023112.html > > [1] http://lists.linbit.com/pipermail/drbd-dev/2016-March/003362.html > > [2] https://forums.grsecurity.net/viewtopic.php?f=3&t=4546 > > [3] https://ubuntuforums.org/showthread.php?t=2336150 > > [4] http://e2.howsolveproblem.com/i/1175162/ > > > > This should go into 4.9, > > and into all stable branches since and including v4.0, > > which is the first to contain the exposing change. > > > > It is correct for all stable branches older than that as well > > (which contain the DRBD driver; which is 2.6.33 and up). > > > > It requires a small "conflict" resolution for v4.4 and earlier, with v4.5 > > we dropped the comment block immediately preceding the kernel_sendmsg(). > > > > Cc: stable@vger.kernel.org > > Cc: viro@zeniv.linux.org.uk > > Cc: christoph.lechleitner@iteg.at > > Cc: wolfgang.glas@iteg.at > > Reported-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> > > Tested-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> > > Signed-off-by: Richard Weinberger <richard@nod.at> > > Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com> > > Changing my patch is perfectly fine, but please clearly state it. > I.e. by adding something like that before your S-o-b. > [Lars: Massaged patch to match my personal taste...] Lars, are you sending a new one? If you do, add the stable tag as well. -- Jens Axboe ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drbd: Fix kernel_sendmsg() usage 2016-11-08 16:52 ` Jens Axboe @ 2016-11-09 15:32 ` Lars Ellenberg 2016-11-09 15:47 ` Richard Weinberger 2016-11-09 16:55 ` Jens Axboe 0 siblings, 2 replies; 13+ messages in thread From: Lars Ellenberg @ 2016-11-09 15:32 UTC (permalink / raw) To: Jens Axboe Cc: Richard Weinberger, wolfgang.glas, christoph.lechleitner, philipp.reisner, stable, linux-kernel, viro, drbd-dev On Tue, Nov 08, 2016 at 09:52:04AM -0700, Jens Axboe wrote: > > > This should go into 4.9, > > > and into all stable branches since and including v4.0, > > > which is the first to contain the exposing change. > > > > > > It is correct for all stable branches older than that as well > > > (which contain the DRBD driver; which is 2.6.33 and up). > > > > > > It requires a small "conflict" resolution for v4.4 and earlier, with v4.5 > > > we dropped the comment block immediately preceding the kernel_sendmsg(). > > > > > > Cc: stable@vger.kernel.org > > > Cc: viro@zeniv.linux.org.uk > > > Cc: christoph.lechleitner@iteg.at > > > Cc: wolfgang.glas@iteg.at > > > Reported-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> > > > Tested-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> > > > Signed-off-by: Richard Weinberger <richard@nod.at> > > > Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com> > > > > Changing my patch is perfectly fine, but please clearly state it. > > I.e. by adding something like that before your S-o-b. > > [Lars: Massaged patch to match my personal taste...] > > Lars, are you sending a new one? If you do, add the stable tag as well. So my "change" against his original patch was - rv = kernel_sendmsg(sock, &msg, &iov, 1, size - sent); + rv = kernel_sendmsg(sock, &msg, &iov, 1, iov.iov_len); to make it "more obviously correct" from looking just at the one line without even having to read the context. And a more verbose commit message. If that requires yet additional noise, sure, so be it :) Should I sent two patches, one that applies to 4.5 and later, and one that applies to 2.6.33 ... 4.4, or are you or stable willing to resolve the trivial "missing comment block" conflict yourself? -- : Lars Ellenberg : LINBIT | Keeping the Digital World Running : DRBD -- Heartbeat -- Corosync -- Pacemaker : R&D, Integration, Ops, Consulting, Support DRBD® and LINBIT® are registered trademarks of LINBIT ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drbd: Fix kernel_sendmsg() usage 2016-11-09 15:32 ` Lars Ellenberg @ 2016-11-09 15:47 ` Richard Weinberger 2016-11-09 16:51 ` [Drbd-dev] " Lars Ellenberg 2016-11-09 16:55 ` Jens Axboe 1 sibling, 1 reply; 13+ messages in thread From: Richard Weinberger @ 2016-11-09 15:47 UTC (permalink / raw) To: Jens Axboe, wolfgang.glas, christoph.lechleitner, philipp.reisner, stable, linux-kernel, viro, drbd-dev On 09.11.2016 16:32, Lars Ellenberg wrote: > On Tue, Nov 08, 2016 at 09:52:04AM -0700, Jens Axboe wrote: >>>> This should go into 4.9, >>>> and into all stable branches since and including v4.0, >>>> which is the first to contain the exposing change. >>>> >>>> It is correct for all stable branches older than that as well >>>> (which contain the DRBD driver; which is 2.6.33 and up). >>>> >>>> It requires a small "conflict" resolution for v4.4 and earlier, with v4.5 >>>> we dropped the comment block immediately preceding the kernel_sendmsg(). >>>> >>>> Cc: stable@vger.kernel.org >>>> Cc: viro@zeniv.linux.org.uk >>>> Cc: christoph.lechleitner@iteg.at >>>> Cc: wolfgang.glas@iteg.at >>>> Reported-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> >>>> Tested-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> >>>> Signed-off-by: Richard Weinberger <richard@nod.at> >>>> Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com> >>> >>> Changing my patch is perfectly fine, but please clearly state it. >>> I.e. by adding something like that before your S-o-b. >>> [Lars: Massaged patch to match my personal taste...] >> > >> Lars, are you sending a new one? If you do, add the stable tag as well. > > So my "change" against his original patch was > - rv = kernel_sendmsg(sock, &msg, &iov, 1, size - sent); > + rv = kernel_sendmsg(sock, &msg, &iov, 1, iov.iov_len); > to make it "more obviously correct" from looking just at the one line > without even having to read the context. And a more verbose commit message. > > If that requires yet additional noise, sure, so be it :) > > Should I sent two patches, one that applies to 4.5 and later, > and one that applies to 2.6.33 ... 4.4, or are you or stable > willing to resolve the trivial "missing comment block" conflict yourself? BTW: Why did you drop the "Fixes:" tag too? Thanks, //richard ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Drbd-dev] [PATCH] drbd: Fix kernel_sendmsg() usage 2016-11-09 15:47 ` Richard Weinberger @ 2016-11-09 16:51 ` Lars Ellenberg 0 siblings, 0 replies; 13+ messages in thread From: Lars Ellenberg @ 2016-11-09 16:51 UTC (permalink / raw) To: Richard Weinberger Cc: Jens Axboe, wolfgang.glas, christoph.lechleitner, philipp.reisner, stable, linux-kernel, viro, drbd-dev On Wed, Nov 09, 2016 at 04:47:15PM +0100, Richard Weinberger wrote: > > Should I sent two patches, one that applies to 4.5 and later, > > and one that applies to 2.6.33 ... 4.4, or are you or stable > > willing to resolve the trivial "missing comment block" conflict yourself? > > BTW: Why did you drop the "Fixes:" tag too? > By accident, probably. I'm ok with you guys just using the original, if you prefer, just let me know. It's the fix, that's important, not how much noise we can make about that oneliner. Lars ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drbd: Fix kernel_sendmsg() usage 2016-11-09 15:32 ` Lars Ellenberg 2016-11-09 15:47 ` Richard Weinberger @ 2016-11-09 16:55 ` Jens Axboe 2016-11-09 21:52 ` [PATCH v2] drbd: Fix kernel_sendmsg() usage - potential NULL deref Lars Ellenberg 1 sibling, 1 reply; 13+ messages in thread From: Jens Axboe @ 2016-11-09 16:55 UTC (permalink / raw) To: Richard Weinberger, wolfgang.glas, christoph.lechleitner, philipp.reisner, stable, linux-kernel, viro, drbd-dev On 11/09/2016 08:32 AM, Lars Ellenberg wrote: > On Tue, Nov 08, 2016 at 09:52:04AM -0700, Jens Axboe wrote: >>>> This should go into 4.9, >>>> and into all stable branches since and including v4.0, >>>> which is the first to contain the exposing change. >>>> >>>> It is correct for all stable branches older than that as well >>>> (which contain the DRBD driver; which is 2.6.33 and up). >>>> >>>> It requires a small "conflict" resolution for v4.4 and earlier, with v4.5 >>>> we dropped the comment block immediately preceding the kernel_sendmsg(). >>>> >>>> Cc: stable@vger.kernel.org >>>> Cc: viro@zeniv.linux.org.uk >>>> Cc: christoph.lechleitner@iteg.at >>>> Cc: wolfgang.glas@iteg.at >>>> Reported-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> >>>> Tested-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> >>>> Signed-off-by: Richard Weinberger <richard@nod.at> >>>> Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com> >>> >>> Changing my patch is perfectly fine, but please clearly state it. >>> I.e. by adding something like that before your S-o-b. >>> [Lars: Massaged patch to match my personal taste...] >> > >> Lars, are you sending a new one? If you do, add the stable tag as well. > > So my "change" against his original patch was > - rv = kernel_sendmsg(sock, &msg, &iov, 1, size - sent); > + rv = kernel_sendmsg(sock, &msg, &iov, 1, iov.iov_len); > to make it "more obviously correct" from looking just at the one line > without even having to read the context. And a more verbose commit message. I'm fine with you making that change, I do that too sometimes for patches. But Richard is absolutely right in that you need to make a note of that. It's no longer the patch he signed off on, so it needs to reflect that. > Should I sent two patches, one that applies to 4.5 and later, > and one that applies to 2.6.33 ... 4.4, or are you or stable > willing to resolve the trivial "missing comment block" conflict yourself? Since it's a trivial one liner, let's just do one for the current series and the stable folks should be able to do that one. If not, they will let us know. -- Jens Axboe ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] drbd: Fix kernel_sendmsg() usage - potential NULL deref 2016-11-09 16:55 ` Jens Axboe @ 2016-11-09 21:52 ` Lars Ellenberg 2016-11-09 23:41 ` Al Viro 0 siblings, 1 reply; 13+ messages in thread From: Lars Ellenberg @ 2016-11-09 21:52 UTC (permalink / raw) To: Jens Axboe Cc: Richard Weinberger, wolfgang.glas, christoph.lechleitner, philipp.reisner, stable, linux-kernel, viro, drbd-dev, Lars Ellenberg From: Richard Weinberger <richard@nod.at> Don't pass a size larger than iov_len to kernel_sendmsg(). Otherwise it will cause a NULL pointer deref when kernel_sendmsg() returns with rv < size. DRBD as external module has been around in the kernel 2.4 days already. We used to be compatible to 2.4 and very early 2.6 kernels, we used to use rv = sock_sendmsg(sock, &msg, iov.iov_len); then later changed to rv = kernel_sendmsg(sock, &msg, &iov, 1, size); when we should have used rv = kernel_sendmsg(sock, &msg, &iov, 1, iov.iov_len); tcp_sendmsg() used to totally ignore the size parameter. 57be5bd ip: convert tcp_sendmsg() to iov_iter primitives changes that, and exposes our long standing error. Even with this error exposed, to trigger the bug, we would need to have an environment (config or otherwise) causing us to not use sendpage() for larger transfers, a failing connection, and have it fail "just at the right time". Apparently that was unlikely enough for most, so this went unnoticed for years. Still, it is known to trigger at least some of these, and suspected for the others: [0] http://lists.linbit.com/pipermail/drbd-user/2016-July/023112.html [1] http://lists.linbit.com/pipermail/drbd-dev/2016-March/003362.html [2] https://forums.grsecurity.net/viewtopic.php?f=3&t=4546 [3] https://ubuntuforums.org/showthread.php?t=2336150 [4] http://e2.howsolveproblem.com/i/1175162/ This should go into 4.9, and into all stable branches since and including v4.0, which is the first to contain the exposing change. It is correct for all stable branches older than that as well (which contain the DRBD driver; which is 2.6.33 and up). It requires a small "conflict" resolution for v4.4 and earlier, with v4.5 we dropped the comment block immediately preceding the kernel_sendmsg(). Fixes: b411b3637fa7 ("The DRBD driver") Cc: <stable@vger.kernel.org> # 2.6.33.x- Cc: viro@zeniv.linux.org.uk Cc: christoph.lechleitner@iteg.at Cc: wolfgang.glas@iteg.at Reported-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> Tested-by: Christoph Lechleitner <christoph.lechleitner@iteg.at> Signed-off-by: Richard Weinberger <richard@nod.at> [changed oneliner to be "obvious" without context; more verbose message] Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com> --- drivers/block/drbd/drbd_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c index 100be55..8348272 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -1871,7 +1871,7 @@ int drbd_send(struct drbd_connection *connection, struct socket *sock, drbd_update_congested(connection); } do { - rv = kernel_sendmsg(sock, &msg, &iov, 1, size); + rv = kernel_sendmsg(sock, &msg, &iov, 1, iov.iov_len); if (rv == -EAGAIN) { if (we_should_drop_the_connection(connection, sock)) break; -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] drbd: Fix kernel_sendmsg() usage - potential NULL deref 2016-11-09 21:52 ` [PATCH v2] drbd: Fix kernel_sendmsg() usage - potential NULL deref Lars Ellenberg @ 2016-11-09 23:41 ` Al Viro 0 siblings, 0 replies; 13+ messages in thread From: Al Viro @ 2016-11-09 23:41 UTC (permalink / raw) To: Lars Ellenberg Cc: Jens Axboe, Richard Weinberger, wolfgang.glas, christoph.lechleitner, philipp.reisner, stable, linux-kernel, drbd-dev On Wed, Nov 09, 2016 at 10:52:58PM +0100, Lars Ellenberg wrote: > This should go into 4.9, > and into all stable branches since and including v4.0, > which is the first to contain the exposing change. > > It is correct for all stable branches older than that as well > (which contain the DRBD driver; which is 2.6.33 and up). > > It requires a small "conflict" resolution for v4.4 and earlier, with v4.5 > we dropped the comment block immediately preceding the kernel_sendmsg(). ACK. I'll rebase commit 7a4992299554 ([drbd] use sock_sendmsg()) on top of that as soon as it hits the mainline. For conspiracy theorists out there (hi, Brad) - that commit (killing the modifications of iovec and reinitializing msg->iov_iter; just set it once and let sendmsg() update it in normal fashion) had been sitting around since late 2014. It happened to fix the bug in question, without a single line refering to that in commit message. Reason: I had completely missed the problem; intent of that loop had been obvious and replacement had obviously done what was intended there. What I had failed to spot was that the code in there did *not* match that intent. Replacement does. And unlike the minimal fix (either version) it doesn't belong in -stable. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drbd: Fix kernel_sendmsg() usage 2016-11-08 10:43 [PATCH] drbd: Fix kernel_sendmsg() usage Richard Weinberger 2016-11-08 13:55 ` Richard Weinberger @ 2016-11-08 14:03 ` Christoph Lechleitner 2016-11-08 15:49 ` Christoph Hellwig 2 siblings, 0 replies; 13+ messages in thread From: Christoph Lechleitner @ 2016-11-08 14:03 UTC (permalink / raw) To: Jens Axboe, linux-kernel, stable, drbd-dev, philipp.reisner, viro, Richard Weinberger, wolfgang.glas Am 2016-11-08 um 14:43 schrieb Lars Ellenberg: > From 3a5859e696178e31a25e65de58c461046fc52beb Mon Sep 17 00:00:00 2001 > From: Richard Weinberger <richard@nod.at> > Date: Tue, 8 Nov 2016 11:43:09 +0100 > Subject: [PATCH] drbd: Fix kernel_sendmsg() usage - potential NULL deref > drbd: Fix kernel_sendmsg() usage - potential NULL deref > Even with this error exposed, to trigger the bug, we would need to have > an environment (config or otherwise) causing us to not use sendpage() > for larger transfers, a flaky connection, and have it fail "just at the > right time". Apparently that was unlikely enough for most, so this went > unnoticed for years. Our drbd configuration was created some 8 years ago. Maybe I should have read more migration tips when upgrading again and again, sorry ;-) But a 30cm Cat6 cable directly connecting 2 dedicated ethernet ports should not match the term "flaky connection". FYI: I co-own the company that hired Richard to track down this bug, that repeatedly (~15 times) forced us to hard-reset servers hosting tens of virtual root servers for our customers. Regards, Christoph Lechleitner ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drbd: Fix kernel_sendmsg() usage 2016-11-08 10:43 [PATCH] drbd: Fix kernel_sendmsg() usage Richard Weinberger 2016-11-08 13:55 ` Richard Weinberger 2016-11-08 14:03 ` [PATCH] drbd: Fix kernel_sendmsg() usage Christoph Lechleitner @ 2016-11-08 15:49 ` Christoph Hellwig 2016-11-08 16:02 ` Richard Weinberger 2016-11-08 16:13 ` Al Viro 2 siblings, 2 replies; 13+ messages in thread From: Christoph Hellwig @ 2016-11-08 15:49 UTC (permalink / raw) To: Richard Weinberger Cc: drbd-dev, linux-kernel, lars.ellenberg, philipp.reisner, stable, viro, christoph.lechleitner, wolfgang.glas On Tue, Nov 08, 2016 at 11:43:09AM +0100, Richard Weinberger wrote: > Don't pass a size larger than iov_len to kernel_sendmsg(). > Otherwise it will cause a NULL pointer deref when kernel_sendmsg() > returns with rv < size. > > Although the issue exists since day 0, only on non-ancient kernels > that contain change 57be5bdad759 ("ip: convert tcp_sendmsg() to iov_iter > primitives") it seems to trigger [0][1][2][3][4]. The real fix here is to convert it to the right primitive, e.g. take Al's patch from here: https://git.kernel.org/cgit/linux/kernel/git/viro/vfs.git/commit/?h=work.sendmsg&id=7a4992299554a9e1ed3c4540bcfa9e40aa9a6376 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drbd: Fix kernel_sendmsg() usage 2016-11-08 15:49 ` Christoph Hellwig @ 2016-11-08 16:02 ` Richard Weinberger 2016-11-08 16:13 ` Al Viro 1 sibling, 0 replies; 13+ messages in thread From: Richard Weinberger @ 2016-11-08 16:02 UTC (permalink / raw) To: Christoph Hellwig Cc: drbd-dev, linux-kernel, lars.ellenberg, philipp.reisner, stable, viro, christoph.lechleitner, wolfgang.glas Christoph, On 08.11.2016 16:49, Christoph Hellwig wrote: > On Tue, Nov 08, 2016 at 11:43:09AM +0100, Richard Weinberger wrote: >> Don't pass a size larger than iov_len to kernel_sendmsg(). >> Otherwise it will cause a NULL pointer deref when kernel_sendmsg() >> returns with rv < size. >> >> Although the issue exists since day 0, only on non-ancient kernels >> that contain change 57be5bdad759 ("ip: convert tcp_sendmsg() to iov_iter >> primitives") it seems to trigger [0][1][2][3][4]. > > The real fix here is to convert it to the right primitive, e.g. take > Al's patch from here: > > https://git.kernel.org/cgit/linux/kernel/git/viro/vfs.git/commit/?h=work.sendmsg&id=7a4992299554a9e1ed3c4540bcfa9e40aa9a6376 Yes, I talked already with Al about this. He suggested to fix the size parameter first such that back-porting to -stable is easy. Thanks, //richard ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drbd: Fix kernel_sendmsg() usage 2016-11-08 15:49 ` Christoph Hellwig 2016-11-08 16:02 ` Richard Weinberger @ 2016-11-08 16:13 ` Al Viro 1 sibling, 0 replies; 13+ messages in thread From: Al Viro @ 2016-11-08 16:13 UTC (permalink / raw) To: Christoph Hellwig Cc: Richard Weinberger, drbd-dev, linux-kernel, lars.ellenberg, philipp.reisner, stable, christoph.lechleitner, wolfgang.glas On Tue, Nov 08, 2016 at 07:49:24AM -0800, Christoph Hellwig wrote: > On Tue, Nov 08, 2016 at 11:43:09AM +0100, Richard Weinberger wrote: > > Don't pass a size larger than iov_len to kernel_sendmsg(). > > Otherwise it will cause a NULL pointer deref when kernel_sendmsg() > > returns with rv < size. > > > > Although the issue exists since day 0, only on non-ancient kernels > > that contain change 57be5bdad759 ("ip: convert tcp_sendmsg() to iov_iter > > primitives") it seems to trigger [0][1][2][3][4]. > > The real fix here is to convert it to the right primitive, e.g. take > Al's patch from here: > > https://git.kernel.org/cgit/linux/kernel/git/viro/vfs.git/commit/?h=work.sendmsg&id=7a4992299554a9e1ed3c4540bcfa9e40aa9a6376 _After_ the backport-friendly part, please. And that's my ACK-by on Richard's variant. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-11-09 23:41 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-08 10:43 [PATCH] drbd: Fix kernel_sendmsg() usage Richard Weinberger 2016-11-08 13:55 ` Richard Weinberger 2016-11-08 16:52 ` Jens Axboe 2016-11-09 15:32 ` Lars Ellenberg 2016-11-09 15:47 ` Richard Weinberger 2016-11-09 16:51 ` [Drbd-dev] " Lars Ellenberg 2016-11-09 16:55 ` Jens Axboe 2016-11-09 21:52 ` [PATCH v2] drbd: Fix kernel_sendmsg() usage - potential NULL deref Lars Ellenberg 2016-11-09 23:41 ` Al Viro 2016-11-08 14:03 ` [PATCH] drbd: Fix kernel_sendmsg() usage Christoph Lechleitner 2016-11-08 15:49 ` Christoph Hellwig 2016-11-08 16:02 ` Richard Weinberger 2016-11-08 16:13 ` 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).