Patchwork [Lucid,Maverick,Natty] SRU: Fix panic after nfs_umount

login
register
mail settings
Submitter Stefan Bader
Date Dec. 9, 2010, 3:58 p.m.
Message ID <4D00FCB1.7090508@canonical.com>
Download mbox | patch
Permalink /patch/74919/
State Superseded
Headers show

Comments

Stefan Bader - Dec. 9, 2010, 3:58 p.m.
SRU justification:

Impact: When trying to mount an export where server and client have no common
authentication method, the client will abort the mount by sending an advisory
unmount message to the server. A bug in the RPC client setup causes the sunrpc
code to access memory outside an allocated array, which will sooner or later
cause the kernel to crash.

Fix: Patch from upstream (about to be submitted and targeted for stable too)
changes the setup to use the actual array size instead of a manually entered number.

Testcase:

Server exports a mount with an authentication method the client does not
support, eg.:
[/etc/exports] /srv/foo *(rw,sec=krb5)

Client tries to mount this directory with no special authentication method:
while true; do mount <server>:/srv/foo /mnt; sync; sleep 1; done

*Note*: This fix is not upstream yet, but is likely to go upstream in that form.
I just wanted to start the SRU process early due to the fact that it triggers
quite easily and ends in an odd and fatal mess. It is obvious enough to me and
has been tested locally.

The change causing the regression has been added in the 2.6.32 time. So all
kernels between that and now are affected.

-Stefan
Tim Gardner - Dec. 9, 2010, 4:14 p.m.
On 12/09/2010 08:58 AM, Stefan Bader wrote:
> SRU justification:
>
> Impact: When trying to mount an export where server and client have no common
> authentication method, the client will abort the mount by sending an advisory
> unmount message to the server. A bug in the RPC client setup causes the sunrpc
> code to access memory outside an allocated array, which will sooner or later
> cause the kernel to crash.
>
> Fix: Patch from upstream (about to be submitted and targeted for stable too)
> changes the setup to use the actual array size instead of a manually entered number.
>
> Testcase:
>
> Server exports a mount with an authentication method the client does not
> support, eg.:
> [/etc/exports] /srv/foo *(rw,sec=krb5)
>
> Client tries to mount this directory with no special authentication method:
> while true; do mount<server>:/srv/foo /mnt; sync; sleep 1; done
>
> *Note*: This fix is not upstream yet, but is likely to go upstream in that form.
> I just wanted to start the SRU process early due to the fact that it triggers
> quite easily and ends in an odd and fatal mess. It is obvious enough to me and
> has been tested locally.
>
> The change causing the regression has been added in the 2.6.32 time. So all
> kernels between that and now are affected.
>
> -Stefan
>

Eeeww! You have to wonder how that ever worked.

Acked-by: Tim Gardner <tim.gardner@canonical.com>
Andy Whitcroft - Dec. 9, 2010, 4:34 p.m.
Applied to Natty.

-apw
Brad Figg - Dec. 9, 2010, 4:56 p.m.
On 12/09/2010 07:58 AM, Stefan Bader wrote:
> SRU justification:
>
> Impact: When trying to mount an export where server and client have no common
> authentication method, the client will abort the mount by sending an advisory
> unmount message to the server. A bug in the RPC client setup causes the sunrpc
> code to access memory outside an allocated array, which will sooner or later
> cause the kernel to crash.
>
> Fix: Patch from upstream (about to be submitted and targeted for stable too)
> changes the setup to use the actual array size instead of a manually entered number.
>
> Testcase:
>
> Server exports a mount with an authentication method the client does not
> support, eg.:
> [/etc/exports] /srv/foo *(rw,sec=krb5)
>
> Client tries to mount this directory with no special authentication method:
> while true; do mount<server>:/srv/foo /mnt; sync; sleep 1; done
>
> *Note*: This fix is not upstream yet, but is likely to go upstream in that form.
> I just wanted to start the SRU process early due to the fact that it triggers
> quite easily and ends in an odd and fatal mess. It is obvious enough to me and
> has been tested locally.
>
> The change causing the regression has been added in the 2.6.32 time. So all
> kernels between that and now are affected.
>
> -Stefan
>

Acked-by: Brad Figg <brad.figg@canonical.com>
Andy Whitcroft - Dec. 9, 2010, 5:27 p.m.
On Thu, Dec 09, 2010 at 04:58:41PM +0100, Stefan Bader wrote:
> SRU justification:
> 
> Impact: When trying to mount an export where server and client have no common
> authentication method, the client will abort the mount by sending an advisory
> unmount message to the server. A bug in the RPC client setup causes the sunrpc
> code to access memory outside an allocated array, which will sooner or later
> cause the kernel to crash.
> 
> Fix: Patch from upstream (about to be submitted and targeted for stable too)
> changes the setup to use the actual array size instead of a manually entered number.
> 
> Testcase:
> 
> Server exports a mount with an authentication method the client does not
> support, eg.:
> [/etc/exports] /srv/foo *(rw,sec=krb5)
> 
> Client tries to mount this directory with no special authentication method:
> while true; do mount <server>:/srv/foo /mnt; sync; sleep 1; done
> 
> *Note*: This fix is not upstream yet, but is likely to go upstream in that form.
> I just wanted to start the SRU process early due to the fact that it triggers
> quite easily and ends in an odd and fatal mess. It is obvious enough to me and
> has been tested locally.
> 
> The change causing the regression has been added in the 2.6.32 time. So all
> kernels between that and now are affected.
> 
> -Stefan

> From dcc0a9d5d9490680ea9faa7b90de3224c3aba7e3 Mon Sep 17 00:00:00 2001
> From: Chuck Lever <chuck.lever@oracle.com>
> Date: Wed, 8 Dec 2010 19:07:12 -0500
> Subject: [PATCH] NFS: Fix panic after nfs_umount()
> 
> After a few unsuccessful NFS mount attempts in which the client and
> server cannot agree on an authentication flavor both support, the
> client panics.  nfs_umount() is invoked in the kernel in this case.
> 
> Turns out this particular UMNT RPC invocation causes the RPC client to
> write off the end of the rpc_clnt's iostat array.  This is because the
> mount client's nrprocs field is initialized with the count of defined
> procedures (two: MNT and UMNT), rather than the size of the client's
> proc array (four).
> 
> The fix is to use the same initialization technique used by most other
> upper layer clients in the kernel.
> 
> Introduced by commit 0b524123, which failed to update nrprocs when it
> added support for UMNT.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=24302
> BugLink: http://bugs.launchpad.net/bugs/683938
> 
> Reported-by: Stefan Bader <stefan.bader@canonical.com>
> Tested-by: Stefan Bader <stefan.bader@canonical.com>
> CC: stable@kernel.org # >= 2.6.32
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/mount_clnt.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/mount_clnt.c b/fs/nfs/mount_clnt.c
> index 59047f8..50552c5 100644
> --- a/fs/nfs/mount_clnt.c
> +++ b/fs/nfs/mount_clnt.c
> @@ -503,13 +503,13 @@ static struct rpc_procinfo mnt3_procedures[] = {
>  
>  static struct rpc_version mnt_version1 = {
>  	.number		= 1,
> -	.nrprocs	= 2,
> +	.nrprocs	= ARRAY_SIZE(mnt_procedures),
>  	.procs		= mnt_procedures,
>  };
>  
>  static struct rpc_version mnt_version3 = {
>  	.number		= 3,
> -	.nrprocs	= 2,
> +	.nrprocs	= ARRAY_SIZE(mnt_procedures),
>  	.procs		= mnt3_procedures,

In this one I think the ARRAY_SIZE should reference mnt3_procedures.  A
quick visual check says that actually they are the same size so the code
will work as it is, but I suspect it is wrong.

Stefan as you have the upstream link could you check.

-apw
Stefan Bader - Dec. 9, 2010, 5:29 p.m.
On 12/09/2010 06:27 PM, Andy Whitcroft wrote:
> On Thu, Dec 09, 2010 at 04:58:41PM +0100, Stefan Bader wrote:
>> SRU justification:
>>
>> Impact: When trying to mount an export where server and client have no common
>> authentication method, the client will abort the mount by sending an advisory
>> unmount message to the server. A bug in the RPC client setup causes the sunrpc
>> code to access memory outside an allocated array, which will sooner or later
>> cause the kernel to crash.
>>
>> Fix: Patch from upstream (about to be submitted and targeted for stable too)
>> changes the setup to use the actual array size instead of a manually entered number.
>>
>> Testcase:
>>
>> Server exports a mount with an authentication method the client does not
>> support, eg.:
>> [/etc/exports] /srv/foo *(rw,sec=krb5)
>>
>> Client tries to mount this directory with no special authentication method:
>> while true; do mount <server>:/srv/foo /mnt; sync; sleep 1; done
>>
>> *Note*: This fix is not upstream yet, but is likely to go upstream in that form.
>> I just wanted to start the SRU process early due to the fact that it triggers
>> quite easily and ends in an odd and fatal mess. It is obvious enough to me and
>> has been tested locally.
>>
>> The change causing the regression has been added in the 2.6.32 time. So all
>> kernels between that and now are affected.
>>
>> -Stefan
> 
>> From dcc0a9d5d9490680ea9faa7b90de3224c3aba7e3 Mon Sep 17 00:00:00 2001
>> From: Chuck Lever <chuck.lever@oracle.com>
>> Date: Wed, 8 Dec 2010 19:07:12 -0500
>> Subject: [PATCH] NFS: Fix panic after nfs_umount()
>>
>> After a few unsuccessful NFS mount attempts in which the client and
>> server cannot agree on an authentication flavor both support, the
>> client panics.  nfs_umount() is invoked in the kernel in this case.
>>
>> Turns out this particular UMNT RPC invocation causes the RPC client to
>> write off the end of the rpc_clnt's iostat array.  This is because the
>> mount client's nrprocs field is initialized with the count of defined
>> procedures (two: MNT and UMNT), rather than the size of the client's
>> proc array (four).
>>
>> The fix is to use the same initialization technique used by most other
>> upper layer clients in the kernel.
>>
>> Introduced by commit 0b524123, which failed to update nrprocs when it
>> added support for UMNT.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=24302
>> BugLink: http://bugs.launchpad.net/bugs/683938
>>
>> Reported-by: Stefan Bader <stefan.bader@canonical.com>
>> Tested-by: Stefan Bader <stefan.bader@canonical.com>
>> CC: stable@kernel.org # >= 2.6.32
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  fs/nfs/mount_clnt.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfs/mount_clnt.c b/fs/nfs/mount_clnt.c
>> index 59047f8..50552c5 100644
>> --- a/fs/nfs/mount_clnt.c
>> +++ b/fs/nfs/mount_clnt.c
>> @@ -503,13 +503,13 @@ static struct rpc_procinfo mnt3_procedures[] = {
>>  
>>  static struct rpc_version mnt_version1 = {
>>  	.number		= 1,
>> -	.nrprocs	= 2,
>> +	.nrprocs	= ARRAY_SIZE(mnt_procedures),
>>  	.procs		= mnt_procedures,
>>  };
>>  
>>  static struct rpc_version mnt_version3 = {
>>  	.number		= 3,
>> -	.nrprocs	= 2,
>> +	.nrprocs	= ARRAY_SIZE(mnt_procedures),
>>  	.procs		= mnt3_procedures,
> 
> In this one I think the ARRAY_SIZE should reference mnt3_procedures.  A
> quick visual check says that actually they are the same size so the code
> will work as it is, but I suspect it is wrong.
> 
> Stefan as you have the upstream link could you check.
> 
> -apw

Yep, my tests were ok for that reason, but Chuck has replied and confirms that
it should be mnt3_procedures.

-Stefan
Ben Hutchings - Dec. 12, 2010, 3:15 a.m.
On Thu, 2010-12-09 at 16:58 +0100, Stefan Bader wrote:
> SRU justification:
> 
> Impact: When trying to mount an export where server and client have no common
> authentication method, the client will abort the mount by sending an advisory
> unmount message to the server. A bug in the RPC client setup causes the sunrpc
> code to access memory outside an allocated array, which will sooner or later
> cause the kernel to crash.
[...]

Do you have a CVE reference for this?

Ben.
Stefan Bader - Dec. 13, 2010, 9:02 a.m.
On 12/12/2010 04:15 AM, Ben Hutchings wrote:
> On Thu, 2010-12-09 at 16:58 +0100, Stefan Bader wrote:
>> SRU justification:
>>
>> Impact: When trying to mount an export where server and client have no common
>> authentication method, the client will abort the mount by sending an advisory
>> unmount message to the server. A bug in the RPC client setup causes the sunrpc
>> code to access memory outside an allocated array, which will sooner or later
>> cause the kernel to crash.
> [...]
> 
> Do you have a CVE reference for this?
> 
> Ben.
> 
Hi Ben,

no it was done as a normal bug afaik. Should we have one?

Stefan
Ben Hutchings - Dec. 15, 2010, 4:47 a.m.
On Mon, 2010-12-13 at 10:02 +0100, Stefan Bader wrote:
> On 12/12/2010 04:15 AM, Ben Hutchings wrote:
> > On Thu, 2010-12-09 at 16:58 +0100, Stefan Bader wrote:
> >> SRU justification:
> >>
> >> Impact: When trying to mount an export where server and client have no common
> >> authentication method, the client will abort the mount by sending an advisory
> >> unmount message to the server. A bug in the RPC client setup causes the sunrpc
> >> code to access memory outside an allocated array, which will sooner or later
> >> cause the kernel to crash.
> > [...]
> > 
> > Do you have a CVE reference for this?
> > 
> > Ben.
> > 
> Hi Ben,
> 
> no it was done as a normal bug afaik. Should we have one?

If I understand correctly, it allows a rogue server to make a client
crash by refusing all its authentication methods.  Obviously this can
also happen without malicious intent, but I don't think that matters.

Ben.
Stefan Bader - Dec. 15, 2010, 10:33 a.m.
On 12/15/2010 05:47 AM, Ben Hutchings wrote:
> On Mon, 2010-12-13 at 10:02 +0100, Stefan Bader wrote:
>> On 12/12/2010 04:15 AM, Ben Hutchings wrote:
>>> On Thu, 2010-12-09 at 16:58 +0100, Stefan Bader wrote:
>>>> SRU justification:
>>>>
>>>> Impact: When trying to mount an export where server and client have no common
>>>> authentication method, the client will abort the mount by sending an advisory
>>>> unmount message to the server. A bug in the RPC client setup causes the sunrpc
>>>> code to access memory outside an allocated array, which will sooner or later
>>>> cause the kernel to crash.
>>> [...]
>>>
>>> Do you have a CVE reference for this?
>>>
>>> Ben.
>>>
>> Hi Ben,
>>
>> no it was done as a normal bug afaik. Should we have one?
> 
> If I understand correctly, it allows a rogue server to make a client
> crash by refusing all its authentication methods.  Obviously this can
> also happen without malicious intent, but I don't think that matters.
> 
> Ben.
> 

Well, it would also require a rogue user (or admin) doing the mount (sometimes
multiple times, sometimes sooner) or to have automount set up to such a server.
Both feels like it needs substantial support from the local side. IIRC someone
from the security team had a look at it and wasn't scared too much. So I was not
either.

Stefan

Patch

From dcc0a9d5d9490680ea9faa7b90de3224c3aba7e3 Mon Sep 17 00:00:00 2001
From: Chuck Lever <chuck.lever@oracle.com>
Date: Wed, 8 Dec 2010 19:07:12 -0500
Subject: [PATCH] NFS: Fix panic after nfs_umount()

After a few unsuccessful NFS mount attempts in which the client and
server cannot agree on an authentication flavor both support, the
client panics.  nfs_umount() is invoked in the kernel in this case.

Turns out this particular UMNT RPC invocation causes the RPC client to
write off the end of the rpc_clnt's iostat array.  This is because the
mount client's nrprocs field is initialized with the count of defined
procedures (two: MNT and UMNT), rather than the size of the client's
proc array (four).

The fix is to use the same initialization technique used by most other
upper layer clients in the kernel.

Introduced by commit 0b524123, which failed to update nrprocs when it
added support for UMNT.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=24302
BugLink: http://bugs.launchpad.net/bugs/683938

Reported-by: Stefan Bader <stefan.bader@canonical.com>
Tested-by: Stefan Bader <stefan.bader@canonical.com>
CC: stable@kernel.org # >= 2.6.32
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/mount_clnt.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/mount_clnt.c b/fs/nfs/mount_clnt.c
index 59047f8..50552c5 100644
--- a/fs/nfs/mount_clnt.c
+++ b/fs/nfs/mount_clnt.c
@@ -503,13 +503,13 @@  static struct rpc_procinfo mnt3_procedures[] = {
 
 static struct rpc_version mnt_version1 = {
 	.number		= 1,
-	.nrprocs	= 2,
+	.nrprocs	= ARRAY_SIZE(mnt_procedures),
 	.procs		= mnt_procedures,
 };
 
 static struct rpc_version mnt_version3 = {
 	.number		= 3,
-	.nrprocs	= 2,
+	.nrprocs	= ARRAY_SIZE(mnt_procedures),
 	.procs		= mnt3_procedures,
 };
 
-- 
1.7.0.4