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: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Reported-by: Christoph Lechleitner <[email protected]>
Tested-by: Christoph Lechleitner <[email protected]>
Signed-off-by: Richard Weinberger <[email protected]>
---
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
Lars,
On 08.11.2016 14:43, Lars Ellenberg wrote:
> From 3a5859e696178e31a25e65de58c461046fc52beb Mon Sep 17 00:00:00 2001
> From: Richard Weinberger <[email protected]>
> 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: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Reported-by: Christoph Lechleitner <[email protected]>
> Tested-by: Christoph Lechleitner <[email protected]>
> Signed-off-by: Richard Weinberger <[email protected]>
> Signed-off-by: Lars Ellenberg <[email protected]>
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
Am 2016-11-08 um 14:43 schrieb Lars Ellenberg:
> From 3a5859e696178e31a25e65de58c461046fc52beb Mon Sep 17 00:00:00 2001
> From: Richard Weinberger <[email protected]>
> 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
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
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
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.
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 <[email protected]>
> > 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: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Reported-by: Christoph Lechleitner <[email protected]>
> > Tested-by: Christoph Lechleitner <[email protected]>
> > Signed-off-by: Richard Weinberger <[email protected]>
> > Signed-off-by: Lars Ellenberg <[email protected]>
>
> 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
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: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Reported-by: Christoph Lechleitner <[email protected]>
> > > Tested-by: Christoph Lechleitner <[email protected]>
> > > Signed-off-by: Richard Weinberger <[email protected]>
> > > Signed-off-by: Lars Ellenberg <[email protected]>
> >
> > 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
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: [email protected]
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Reported-by: Christoph Lechleitner <[email protected]>
>>>> Tested-by: Christoph Lechleitner <[email protected]>
>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>> Signed-off-by: Lars Ellenberg <[email protected]>
>>>
>>> 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
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
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: [email protected]
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Reported-by: Christoph Lechleitner <[email protected]>
>>>> Tested-by: Christoph Lechleitner <[email protected]>
>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>> Signed-off-by: Lars Ellenberg <[email protected]>
>>>
>>> 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
From: Richard Weinberger <[email protected]>
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: <[email protected]> # 2.6.33.x-
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Reported-by: Christoph Lechleitner <[email protected]>
Tested-by: Christoph Lechleitner <[email protected]>
Signed-off-by: Richard Weinberger <[email protected]>
[changed oneliner to be "obvious" without context; more verbose message]
Signed-off-by: Lars Ellenberg <[email protected]>
---
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
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.