diff mbox

RDS: Fix rds-ping inducing kernel panic

Message ID 20180122112415.GA41074@beast (mailing list archive)
State Not Applicable
Headers show

Commit Message

Kees Cook Jan. 22, 2018, 11:24 a.m. UTC
As described in: https://bugzilla.redhat.com/show_bug.cgi?id=822754

Attempting an RDS connection from the IP address of an IPoIB interface
to itself causes a kernel panic due to a BUG_ON() being triggered.
Making the test less strict allows rds-ping to work without crashing
the machine.

A local unprivileged user could use this flaw to crash the sytem.

I think this fix was written by Jay Fenlason <fenlason@redhat.com>,
and extracted from the RedHat kernel patches here:

https://oss.oracle.com/git/gitweb.cgi?p=redpatch.git;a=commitdiff;h=c7b6a0a1d8d636852be130fa15fa8be10d4704e8

This fix appears to have been carried by at least RedHat, Oracle, and
Ubuntu for several years.

CVE-2012-2372

Reported-by: Honggang Li <honli@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
This is what I get for researching CVE lifetimes...
---
 net/rds/ib_send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Leon Romanovsky Jan. 22, 2018, 3:10 p.m. UTC | #1
On Mon, Jan 22, 2018 at 03:24:15AM -0800, Kees Cook wrote:
> As described in: https://bugzilla.redhat.com/show_bug.cgi?id=822754
>
> Attempting an RDS connection from the IP address of an IPoIB interface
> to itself causes a kernel panic due to a BUG_ON() being triggered.
> Making the test less strict allows rds-ping to work without crashing
> the machine.
>
> A local unprivileged user could use this flaw to crash the sytem.

s/sytem/system

>
> I think this fix was written by Jay Fenlason <fenlason@redhat.com>,
> and extracted from the RedHat kernel patches here:
>
> https://oss.oracle.com/git/gitweb.cgi?p=redpatch.git;a=commitdiff;h=c7b6a0a1d8d636852be130fa15fa8be10d4704e8
>
> This fix appears to have been carried by at least RedHat, Oracle, and
> Ubuntu for several years.
>
> CVE-2012-2372
>
> Reported-by: Honggang Li <honli@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> This is what I get for researching CVE lifetimes...
> ---
>  net/rds/ib_send.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
> index 8557a1cae041..5fbf635d17cb 100644
> --- a/net/rds/ib_send.c
> +++ b/net/rds/ib_send.c
> @@ -506,7 +506,7 @@ int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
>  	int flow_controlled = 0;
>  	int nr_sig = 0;
>
> -	BUG_ON(off % RDS_FRAG_SIZE);
> +	BUG_ON(!conn->c_loopback && off % RDS_FRAG_SIZE);
>  	BUG_ON(hdr_off != 0 && hdr_off != sizeof(struct rds_header));

To be honest this function full of BUG_ONs and it looks fishy to have them there.
Why don't we return EINVAL instead of crashing system?

Thanks

>
>  	/* Do not send cong updates to IB loopback */
> --
> 2.7.4
>
>
> --
> Kees Cook
> Pixel Security
David Miller Jan. 22, 2018, 3:47 p.m. UTC | #2
From: Leon Romanovsky <leon@kernel.org>
Date: Mon, 22 Jan 2018 17:10:54 +0200

> On Mon, Jan 22, 2018 at 03:24:15AM -0800, Kees Cook wrote:
>> diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
>> index 8557a1cae041..5fbf635d17cb 100644
>> --- a/net/rds/ib_send.c
>> +++ b/net/rds/ib_send.c
>> @@ -506,7 +506,7 @@ int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
>>  	int flow_controlled = 0;
>>  	int nr_sig = 0;
>>
>> -	BUG_ON(off % RDS_FRAG_SIZE);
>> +	BUG_ON(!conn->c_loopback && off % RDS_FRAG_SIZE);
>>  	BUG_ON(hdr_off != 0 && hdr_off != sizeof(struct rds_header));
> 
> To be honest this function full of BUG_ONs and it looks fishy to have them there.
> Why don't we return EINVAL instead of crashing system?

I completely agree that these assertions should just cause an error-out
rather than trigger a BUG().
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Santosh Shilimkar Jan. 22, 2018, 5:01 p.m. UTC | #3
On 1/22/2018 3:24 AM, Kees Cook wrote:
> As described in: https://bugzilla.redhat.com/show_bug.cgi?id=822754
> 
> Attempting an RDS connection from the IP address of an IPoIB interface
> to itself causes a kernel panic due to a BUG_ON() being triggered.
> Making the test less strict allows rds-ping to work without crashing
> the machine.
> 
> A local unprivileged user could use this flaw to crash the sytem.
>
Are you able to reproduce this issue on mainline kernel ?
IIRC, this sjouldn't happen anymore but if you see it, please
let me know. Will try it as well. rds-ping on self
loopback device is often tested and used as well for
monitoring services in production.

> I think this fix was written by Jay Fenlason <fenlason@redhat.com>,
> and extracted from the RedHat kernel patches here:
> 
> https://oss.oracle.com/git/gitweb.cgi?p=redpatch.git;a=commitdiff;h=c7b6a0a1d8d636852be130fa15fa8be10d4704e8
>
It was part of redhat patched kernel but not carried in shipping
Oracle UEK kernels at least afaik.

> This fix appears to have been carried by at least RedHat, Oracle, and
> Ubuntu for several years.
>
> CVE-2012-2372
> 
> Reported-by: Honggang Li <honli@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> This is what I get for researching CVE lifetimes...
Am not sure if its applicable anymore. Infact the issue with
loopback device was due to congestion update and thats been
already addressed with commit '18fc25c94: {rds: prevent BUG_ON
triggered on congestion update to loopback}'

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Santosh Shilimkar Jan. 22, 2018, 5:04 p.m. UTC | #4
On 1/22/2018 7:47 AM, David Miller wrote:
> From: Leon Romanovsky <leon@kernel.org>
> Date: Mon, 22 Jan 2018 17:10:54 +0200
> 
>> On Mon, Jan 22, 2018 at 03:24:15AM -0800, Kees Cook wrote:
>>> diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
>>> index 8557a1cae041..5fbf635d17cb 100644
>>> --- a/net/rds/ib_send.c
>>> +++ b/net/rds/ib_send.c
>>> @@ -506,7 +506,7 @@ int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
>>>   	int flow_controlled = 0;
>>>   	int nr_sig = 0;
>>>
>>> -	BUG_ON(off % RDS_FRAG_SIZE);
>>> +	BUG_ON(!conn->c_loopback && off % RDS_FRAG_SIZE);
>>>   	BUG_ON(hdr_off != 0 && hdr_off != sizeof(struct rds_header));
>>
>> To be honest this function full of BUG_ONs and it looks fishy to have them there.
>> Why don't we return EINVAL instead of crashing system?
> 
> I completely agree that these assertions should just cause an error-out
> rather than trigger a BUG().

Andy did remove bunch of them but there are still few more left overs.
Will have a look at remainder set since most of them were added during
early development and remained there. Thanks Dave/Leon.

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook Jan. 22, 2018, 10:17 p.m. UTC | #5
On Tue, Jan 23, 2018 at 4:01 AM, Santosh Shilimkar
<santosh.shilimkar@oracle.com> wrote:
> On 1/22/2018 3:24 AM, Kees Cook wrote:
>>
>> As described in: https://bugzilla.redhat.com/show_bug.cgi?id=822754
>>
>> Attempting an RDS connection from the IP address of an IPoIB interface
>> to itself causes a kernel panic due to a BUG_ON() being triggered.
>> Making the test less strict allows rds-ping to work without crashing
>> the machine.
>>
>> A local unprivileged user could use this flaw to crash the sytem.
>>
> Are you able to reproduce this issue on mainline kernel ?
> IIRC, this sjouldn't happen anymore but if you see it, please
> let me know. Will try it as well. rds-ping on self
> loopback device is often tested and used as well for
> monitoring services in production.

I don't have an RDS test setup, no. But it sounds like kernels without
this patch aren't seeing the problem.

> Am not sure if its applicable anymore. Infact the issue with
> loopback device was due to congestion update and thats been
> already addressed with commit '18fc25c94: {rds: prevent BUG_ON
> triggered on congestion update to loopback}'

That looks very much like it was fixed there. Thanks!

-Kees
Santosh Shilimkar Jan. 22, 2018, 10:25 p.m. UTC | #6
On 1/22/2018 2:17 PM, Kees Cook wrote:
> On Tue, Jan 23, 2018 at 4:01 AM, Santosh Shilimkar
> <santosh.shilimkar@oracle.com> wrote:
>> On 1/22/2018 3:24 AM, Kees Cook wrote:
>>>
>>> As described in: https://bugzilla.redhat.com/show_bug.cgi?id=822754
>>>
>>> Attempting an RDS connection from the IP address of an IPoIB interface
>>> to itself causes a kernel panic due to a BUG_ON() being triggered.
>>> Making the test less strict allows rds-ping to work without crashing
>>> the machine.
>>>
>>> A local unprivileged user could use this flaw to crash the sytem.
>>>
>> Are you able to reproduce this issue on mainline kernel ?
>> IIRC, this sjouldn't happen anymore but if you see it, please
>> let me know. Will try it as well. rds-ping on self
>> loopback device is often tested and used as well for
>> monitoring services in production.
> 
> I don't have an RDS test setup, no. But it sounds like kernels without
> this patch aren't seeing the problem.
>
Yep. Thats what I thought and hence asked.

>> Am not sure if its applicable anymore. Infact the issue with
>> loopback device was due to congestion update and thats been
>> already addressed with commit '18fc25c94: {rds: prevent BUG_ON
>> triggered on congestion update to loopback}'
> 
> That looks very much like it was fixed there. Thanks!
> 
Yeah. Thanks Kees !!

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
index 8557a1cae041..5fbf635d17cb 100644
--- a/net/rds/ib_send.c
+++ b/net/rds/ib_send.c
@@ -506,7 +506,7 @@  int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
 	int flow_controlled = 0;
 	int nr_sig = 0;
 
-	BUG_ON(off % RDS_FRAG_SIZE);
+	BUG_ON(!conn->c_loopback && off % RDS_FRAG_SIZE);
 	BUG_ON(hdr_off != 0 && hdr_off != sizeof(struct rds_header));
 
 	/* Do not send cong updates to IB loopback */