[PATCH] nfs client, kernel 2.4.31: readlink result overflow

[PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Assar]

In 2.4.31, the v2/3 nfs readlink accepts too long symlinks.
I have tested this by having a server return long symlinks.
2.6.13 does not to my reading have this problem.

diff -u linux-2.4.31.orig/fs/nfs/nfs2xdr.c linux-2.4.31/fs/nfs/nfs2xdr.c
--- linux-2.4.31.orig/fs/nfs/nfs2xdr.c	2002-11-28 18:53:15.000000000 -0500
+++ linux-2.4.31/fs/nfs/nfs2xdr.c	2005-09-07 17:36:04.000000000 -0400
@@ -571,8 +571,8 @@
 	strlen = (u32*)kmap(rcvbuf->pages[0]);
 	/* Convert length of symlink */
 	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len)
-		len = rcvbuf->page_len;
+	if (len > rcvbuf->page_len - 1 - 4)
+		len = rcvbuf->page_len - 1 - 4;
 	*strlen = len;
 	/* NULL terminate the string we got */
 	string = (char *)(strlen + 1);
diff -u linux-2.4.31.orig/fs/nfs/nfs3xdr.c linux-2.4.31/fs/nfs/nfs3xdr.c
--- linux-2.4.31.orig/fs/nfs/nfs3xdr.c	2003-11-28 13:26:21.000000000 -0500
+++ linux-2.4.31/fs/nfs/nfs3xdr.c	2005-09-07 17:53:10.000000000 -0400
@@ -759,8 +759,8 @@
 	strlen = (u32*)kmap(rcvbuf->pages[0]);
 	/* Convert length of symlink */
 	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len)
-		len = rcvbuf->page_len;
+	if (len > rcvbuf->page_len - 1 - 4)
+		len = rcvbuf->page_len - 1 - 4;
 	*strlen = len;
 	/* NULL terminate the string we got */
 	string = (char *)(strlen + 1);

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

On Mon, 12 Sep 2005 09:26:28 EDT, Assar said:
> In 2.4.31, the v2/3 nfs readlink accepts too long symlinks.
> I have tested this by having a server return long symlinks.
> 2.6.13 does not to my reading have this problem.

Odd, as it has similar code - 2.6.13-mm1 nfs2xdr.c has:
        /* Convert length of symlink */
        len = ntohl(*p++);
        if (len >= rcvbuf->page_len || len <= 0) {

Is there some *other* code in 2.6 that prevents this from being a problem,
if it's a problem on 2.4?

> diff -u linux-2.4.31.orig/fs/nfs/nfs2xdr.c linux-2.4.31/fs/nfs/nfs2xdr.c
> --- linux-2.4.31.orig/fs/nfs/nfs2xdr.c	2002-11-28 18:53:15.000000000 -0500
> +++ linux-2.4.31/fs/nfs/nfs2xdr.c	2005-09-07 17:36:04.000000000 -0400
> @@ -571,8 +571,8 @@
>  	strlen = (u32*)kmap(rcvbuf->pages[0]);
>  	/* Convert length of symlink */
>  	len = ntohl(*strlen);
> -	if (len > rcvbuf->page_len)
> -		len = rcvbuf->page_len;
> +	if (len > rcvbuf->page_len - 1 - 4)
> +		len = rcvbuf->page_len - 1 - 4;

Am I the only one who finds an uncommented "-1 -4" construct scary beyond belief?


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Assar]

Valdis.Kletnieks@vt.edu writes:
> Odd, as it has similar code - 2.6.13-mm1 nfs2xdr.c has:
>         /* Convert length of symlink */
>         len = ntohl(*p++);
>         if (len >= rcvbuf->page_len || len <= 0) {

To my reading, the 2.6.13 code does not copy the 4 bytes of length to
rcvbuf.

> Is there some *other* code in 2.6 that prevents this from being a problem,
> if it's a problem on 2.4?
> 
> > diff -u linux-2.4.31.orig/fs/nfs/nfs2xdr.c linux-2.4.31/fs/nfs/nfs2xdr.c
> > --- linux-2.4.31.orig/fs/nfs/nfs2xdr.c	2002-11-28 18:53:15.000000000 -0500
> > +++ linux-2.4.31/fs/nfs/nfs2xdr.c	2005-09-07 17:36:04.000000000 -0400
> > @@ -571,8 +571,8 @@
> >  	strlen = (u32*)kmap(rcvbuf->pages[0]);
> >  	/* Convert length of symlink */
> >  	len = ntohl(*strlen);
> > -	if (len > rcvbuf->page_len)
> > -		len = rcvbuf->page_len;
> > +	if (len > rcvbuf->page_len - 1 - 4)
> > +		len = rcvbuf->page_len - 1 - 4;
> 
> Am I the only one who finds an uncommented "-1 -4" construct scary beyond belief?

Probably not.  What do you think looks better?  sizeof(u32) ?

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

On Mon, 12 Sep 2005 15:37:46 EDT, Assar said:
> Valdis.Kletnieks@vt.edu writes:
> > Odd, as it has similar code - 2.6.13-mm1 nfs2xdr.c has:
> >         /* Convert length of symlink */
> >         len = ntohl(*p++);
> >         if (len >= rcvbuf->page_len || len <= 0) {
> 
> To my reading, the 2.6.13 code does not copy the 4 bytes of length to
> rcvbuf.

Hmm... it still does this:
	kaddr[len+rcvbuf->page_base] = '\0';
which still has a possible off-by-one? (Was that why you have -1 -4?)

> > Am I the only one who finds an uncommented "-1 -4" construct scary beyond belief?
> 
> Probably not.  What do you think looks better?  sizeof(u32) ?

sizeof(actual_var) is even better, as that way it's clear what you're allowing
space for.


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Assar]

Valdis.Kletnieks@vt.edu writes:
> > To my reading, the 2.6.13 code does not copy the 4 bytes of length to
> > rcvbuf.
> 
> Hmm... it still does this:
> 	kaddr[len+rcvbuf->page_base] = '\0';
> which still has a possible off-by-one? (Was that why you have -1 -4?)

The check is different.  2.6.13 is using ">=" instead of ">", so hence
I think that's fine.

> sizeof(actual_var) is even better, as that way it's clear what you're allowing
> space for.

diff -u linux-2.4.31.orig/fs/nfs/nfs2xdr.c linux-2.4.31/fs/nfs/nfs2xdr.c
--- linux-2.4.31.orig/fs/nfs/nfs2xdr.c	2002-11-28 18:53:15.000000000 -0500
+++ linux-2.4.31/fs/nfs/nfs2xdr.c	2005-09-12 16:12:30.000000000 -0400
@@ -571,8 +571,8 @@
 	strlen = (u32*)kmap(rcvbuf->pages[0]);
 	/* Convert length of symlink */
 	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len)
-		len = rcvbuf->page_len;
+	if (len > rcvbuf->page_len - sizeof(*strlen) - 1)
+		len = rcvbuf->page_len - sizeof(*strlen) - 1;
 	*strlen = len;
 	/* NULL terminate the string we got */
 	string = (char *)(strlen + 1);
diff -u linux-2.4.31.orig/fs/nfs/nfs3xdr.c linux-2.4.31/fs/nfs/nfs3xdr.c
--- linux-2.4.31.orig/fs/nfs/nfs3xdr.c	2003-11-28 13:26:21.000000000 -0500
+++ linux-2.4.31/fs/nfs/nfs3xdr.c	2005-09-12 16:12:29.000000000 -0400
@@ -759,8 +759,8 @@
 	strlen = (u32*)kmap(rcvbuf->pages[0]);
 	/* Convert length of symlink */
 	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len)
-		len = rcvbuf->page_len;
+	if (len > rcvbuf->page_len - sizeof(*strlen) - 1)
+		len = rcvbuf->page_len - sizeof(*strlen) - 1;
 	*strlen = len;
 	/* NULL terminate the string we got */
 	string = (char *)(strlen + 1);

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

On Mon, 12 Sep 2005 16:41:19 EDT, Assar said:
> Valdis.Kletnieks@vt.edu writes:
> > > To my reading, the 2.6.13 code does not copy the 4 bytes of length to
> > > rcvbuf.
> > 
> > Hmm... it still does this:
> > 	kaddr[len+rcvbuf->page_base] = '\0';
> > which still has a possible off-by-one? (Was that why you have -1 -4?)
> 
> The check is different.  2.6.13 is using ">=" instead of ">", so hence
> I think that's fine.

Argh.  Where's my reading glasses? ;)  Yes, a >= check works there....

> +	if (len > rcvbuf->page_len - sizeof(*strlen) - 1)
> +		len = rcvbuf->page_len - sizeof(*strlen) - 1;

OK, looks saner to me, but Trond and Marcelo probably get to make the final decision ;)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Marcelo Tosatti]

Hi Assar,

On Mon, Sep 12, 2005 at 04:41:19PM -0400, Assar wrote:
> Valdis.Kletnieks@vt.edu writes:
> > > To my reading, the 2.6.13 code does not copy the 4 bytes of length to
> > > rcvbuf.
> > 
> > Hmm... it still does this:
> > 	kaddr[len+rcvbuf->page_base] = '\0';
> > which still has a possible off-by-one? (Was that why you have -1 -4?)
> 
> The check is different.  2.6.13 is using ">=" instead of ">", so hence
> I think that's fine.
> 
> > sizeof(actual_var) is even better, as that way it's clear what you're allowing
> > space for.
> 
> diff -u linux-2.4.31.orig/fs/nfs/nfs2xdr.c linux-2.4.31/fs/nfs/nfs2xdr.c
> --- linux-2.4.31.orig/fs/nfs/nfs2xdr.c	2002-11-28 18:53:15.000000000 -0500
> +++ linux-2.4.31/fs/nfs/nfs2xdr.c	2005-09-12 16:12:30.000000000 -0400
> @@ -571,8 +571,8 @@
>  	strlen = (u32*)kmap(rcvbuf->pages[0]);
>  	/* Convert length of symlink */
>  	len = ntohl(*strlen);
> -	if (len > rcvbuf->page_len)
> -		len = rcvbuf->page_len;
> +	if (len > rcvbuf->page_len - sizeof(*strlen) - 1)
> +		len = rcvbuf->page_len - sizeof(*strlen) - 1;

So the problem is that the "len" variable encapsulated in (u32 *)rcvbuf->pages[0]
does not account for its own length (4 bytes)? 

If thats the reason, you don't need the "-1" there?

Someone with better understanding to ACK this would be nice. Trond?

>  	*strlen = len;
>  	/* NULL terminate the string we got */
>  	string = (char *)(strlen + 1);
> diff -u linux-2.4.31.orig/fs/nfs/nfs3xdr.c linux-2.4.31/fs/nfs/nfs3xdr.c
> --- linux-2.4.31.orig/fs/nfs/nfs3xdr.c	2003-11-28 13:26:21.000000000 -0500
> +++ linux-2.4.31/fs/nfs/nfs3xdr.c	2005-09-12 16:12:29.000000000 -0400
> @@ -759,8 +759,8 @@
>  	strlen = (u32*)kmap(rcvbuf->pages[0]);
>  	/* Convert length of symlink */
>  	len = ntohl(*strlen);
> -	if (len > rcvbuf->page_len)
> -		len = rcvbuf->page_len;
> +	if (len > rcvbuf->page_len - sizeof(*strlen) - 1)
> +		len = rcvbuf->page_len - sizeof(*strlen) - 1;
>  	*strlen = len;
>  	/* NULL terminate the string we got */
>  	string = (char *)(strlen + 1);

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Assar]

Hi, Marcelo.

Marcelo Tosatti <marcelo.tosatti@cyclades.com> writes:
> > diff -u linux-2.4.31.orig/fs/nfs/nfs2xdr.c linux-2.4.31/fs/nfs/nfs2xdr.c
> > --- linux-2.4.31.orig/fs/nfs/nfs2xdr.c	2002-11-28 18:53:15.000000000 -0500
> > +++ linux-2.4.31/fs/nfs/nfs2xdr.c	2005-09-12 16:12:30.000000000 -0400
> > @@ -571,8 +571,8 @@
> >  	strlen = (u32*)kmap(rcvbuf->pages[0]);
> >  	/* Convert length of symlink */
> >  	len = ntohl(*strlen);
> > -	if (len > rcvbuf->page_len)
> > -		len = rcvbuf->page_len;
> > +	if (len > rcvbuf->page_len - sizeof(*strlen) - 1)
> > +		len = rcvbuf->page_len - sizeof(*strlen) - 1;
> 
> So the problem is that the "len" variable encapsulated in (u32 *)rcvbuf->pages[0]
> does not account for its own length (4 bytes)? 

That's one problem.

> If thats the reason, you don't need the "-1" there?

It also writes a 0 byte.  I think it looks like this:

---- ------------ -
len  string...    0

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Marcelo Tosatti]

On Tue, Sep 13, 2005 at 02:52:44PM -0400, Assar wrote:
> Hi, Marcelo.
> 
> Marcelo Tosatti <marcelo.tosatti@cyclades.com> writes:
> > > diff -u linux-2.4.31.orig/fs/nfs/nfs2xdr.c linux-2.4.31/fs/nfs/nfs2xdr.c
> > > --- linux-2.4.31.orig/fs/nfs/nfs2xdr.c	2002-11-28 18:53:15.000000000 -0500
> > > +++ linux-2.4.31/fs/nfs/nfs2xdr.c	2005-09-12 16:12:30.000000000 -0400
> > > @@ -571,8 +571,8 @@
> > >  	strlen = (u32*)kmap(rcvbuf->pages[0]);
> > >  	/* Convert length of symlink */
> > >  	len = ntohl(*strlen);
> > > -	if (len > rcvbuf->page_len)
> > > -		len = rcvbuf->page_len;
> > > +	if (len > rcvbuf->page_len - sizeof(*strlen) - 1)
> > > +		len = rcvbuf->page_len - sizeof(*strlen) - 1;
> > 
> > So the problem is that the "len" variable encapsulated in (u32 *)rcvbuf->pages[0]
> > does not account for its own length (4 bytes)? 
> 
> That's one problem.
> 
> > If thats the reason, you don't need the "-1" there?
> 
> It also writes a 0 byte.  I think it looks like this:
> 
> ---- ------------ -
> len  string...    0

If an overflow happens (len > rcvbuf->page_len) the last character will get 
truncated anyway, so there is no need for the "-1" AFAICS.

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Assar]

Marcelo Tosatti <marcelo.tosatti@cyclades.com> writes:
> > That's one problem.
> > 
> > > If thats the reason, you don't need the "-1" there?
> > 
> > It also writes a 0 byte.  I think it looks like this:
> > 
> > ---- ------------ -
> > len  string...    0
> 
> If an overflow happens (len > rcvbuf->page_len) the last character will get 
> truncated anyway, so there is no need for the "-1" AFAICS.

I'm not sure I follow.

The code writes a 0 at rcvbuf->pages[0][sizeof(u32) + len], right?
Doesn't that make the maximum allowed value of len should be
'rcvbuf->page_len - sizeof(u32) - 1' ?

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Peter Staubach]

Assar wrote:

>>If thats the reason, you don't need the "-1" there?
>>    
>>
>
>It also writes a 0 byte.  I think it looks like this:
>
>---- ------------ -
>len  string...    0
>
>-
>

NFS uses XDR to encode C strings.  They are encoded as counted byte arrays
and are _not_ null terminated.  The space containing the string is rounded
up to the next 4 byte boundary though and, usually, this space is zero 
filled.
The number of bytes in the string is encoded as a big endian integer in the
first four bytes.

    Thanx...

       ps

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Assar]

Peter Staubach <staubach@redhat.com> writes:
> NFS uses XDR to encode C strings.  They are encoded as counted byte arrays
> and are _not_ null terminated.  The space containing the string is rounded
> up to the next 4 byte boundary though and, usually, this space is zero
> filled.
> The number of bytes in the string is encoded as a big endian integer in the
> first four bytes.

Yes, but fs/nfs/nfs2xdr.c:nfs_xdr_readlinkres on 2.4.31 writes a 0 at
the end of string after having received it, which is what started this
thread.  Look at the end of nfs_xdr_readlinkres.

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Peter Staubach]

Assar wrote:

 >Peter Staubach <staubach@redhat.com> writes:
 >
 >>>Yes, but fs/nfs/nfs2xdr.c:nfs_xdr_readlinkres on 2.4.31 writes a 0 at
 >>>the end of string after having received it, which is what started this
 >>>thread.  Look at the end of nfs_xdr_readlinkres.
 >>>
 >>Yes, I know that.  For C purposes, the string must be null terminated.
 >
 >
 >Then I'm sorry but I don't understand what your point was.  Do you
 >believe there's a bug in nfs_xdr_readlinkres and if so, how do you
 >think it should work?
 >

Yes, I think that there is a bug in the boundary checking.  I think that:

        if (len > rcvbuf->page_len)

should be

        if (len >= rcvbuf->page_len - sizeof(u32) || len > 1024)

because the code puts the length in the first 4 bytes and then the
contents of the symbolic link is stored in the rest of the page.
The ">=" accounts for the null byte will be appended to the length.
The additional check for 1024 is due to the NFS Version 2 protocol
limiting the size of the contents of a symbolic link which can be
returned to 1024 bytes.

Also, the NFS Version 3, nfs3_xdr_readlinkres, is broken in the same
way and will need to be changed in the same fashion, except that
the NFS Version 3 protocol does not place an arbitrary limit on the
size of the contents of the symbolic which can be returned.  The
comparison against 1024 won't be needed here.

--

The 2.6 kernel code is also broken, but in a different, but once again,
similar fashions.

       ps

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Assar]

Peter Staubach <staubach@redhat.com> writes:
> Yes, I think that there is a bug in the boundary checking.  I think that:
> 
>         if (len > rcvbuf->page_len)
> 
> should be
> 
>         if (len >= rcvbuf->page_len - sizeof(u32) || len > 1024)

Thanks for your feedback.  The patch to 2.4.31 that incorporates your
suggsted changes is here:

diff -u linux-2.4.31.orig/fs/nfs/nfs2xdr.c linux-2.4.31/fs/nfs/nfs2xdr.c
--- linux-2.4.31.orig/fs/nfs/nfs2xdr.c	2002-11-28 18:53:15.000000000 -0500
+++ linux-2.4.31/fs/nfs/nfs2xdr.c	2005-09-14 15:33:30.000000000 -0400
@@ -571,8 +571,8 @@
 	strlen = (u32*)kmap(rcvbuf->pages[0]);
 	/* Convert length of symlink */
 	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len)
-		len = rcvbuf->page_len;
+	if (len >= rcvbuf->page_len - sizeof(u32) || len > NFS2_MAXPATHLEN)
+		len = rcvbuf->page_len - sizeof(u32) - 1;
 	*strlen = len;
 	/* NULL terminate the string we got */
 	string = (char *)(strlen + 1);
diff -u linux-2.4.31.orig/fs/nfs/nfs3xdr.c linux-2.4.31/fs/nfs/nfs3xdr.c
--- linux-2.4.31.orig/fs/nfs/nfs3xdr.c	2003-11-28 13:26:21.000000000 -0500
+++ linux-2.4.31/fs/nfs/nfs3xdr.c	2005-09-14 15:33:53.000000000 -0400
@@ -759,8 +759,8 @@
 	strlen = (u32*)kmap(rcvbuf->pages[0]);
 	/* Convert length of symlink */
 	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len)
-		len = rcvbuf->page_len;
+	if (len >= rcvbuf->page_len - sizeof(u32))
+		len = rcvbuf->page_len - sizeof(u32) - 1;
 	*strlen = len;
 	/* NULL terminate the string we got */
 	string = (char *)(strlen + 1);

> The 2.6 kernel code is also broken, but in a different, but once again,
> similar fashions.

You mean for length > 1024 ?

diff -u linux-2.6.13.orig/fs/nfs/nfs2xdr.c linux-2.6.13/fs/nfs/nfs2xdr.c
--- linux-2.6.13.orig/fs/nfs/nfs2xdr.c	2005-08-28 19:41:01.000000000 -0400
+++ linux-2.6.13/fs/nfs/nfs2xdr.c	2005-09-14 15:40:13.000000000 -0400
@@ -557,7 +557,7 @@
 		return -nfs_stat_to_errno(status);
 	/* Convert length of symlink */
 	len = ntohl(*p++);
-	if (len >= rcvbuf->page_len || len <= 0) {
+	if (len >= rcvbuf->page_len || len <= 0 || len > NFS2_MAXPATHLEN) {
 		dprintk(KERN_WARNING "nfs: server returned giant symlink!\n");
 		return -ENAMETOOLONG;
 	}

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Peter Staubach]

Assar wrote:

>>The 2.6 kernel code is also broken, but in a different, but once again,
>>similar fashions.
>>    
>>
>
>You mean for length > 1024 ?
>
>diff -u linux-2.6.13.orig/fs/nfs/nfs2xdr.c linux-2.6.13/fs/nfs/nfs2xdr.c
>--- linux-2.6.13.orig/fs/nfs/nfs2xdr.c	2005-08-28 19:41:01.000000000 -0400
>+++ linux-2.6.13/fs/nfs/nfs2xdr.c	2005-09-14 15:40:13.000000000 -0400
>@@ -557,7 +557,7 @@
> 		return -nfs_stat_to_errno(status);
> 	/* Convert length of symlink */
> 	len = ntohl(*p++);
>-	if (len >= rcvbuf->page_len || len <= 0) {
>+	if (len >= rcvbuf->page_len || len <= 0 || len > NFS2_MAXPATHLEN) {
> 		dprintk(KERN_WARNING "nfs: server returned giant symlink!\n");
> 		return -ENAMETOOLONG;
> 	}
>

This code appears to assume that rcvbuf->page_base is zero here, but then
uses rcvbuf->page_base when calculating where to place the null byte.  It
seems to me that it should either use rcvbuf->page_base in both
calculations or neither.

    Thanx...

       ps

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Peter Staubach]

Assar wrote:

>Peter Staubach <staubach@redhat.com> writes:
>  
>
>>Yes, I think that there is a bug in the boundary checking.  I think that:
>>
>>        if (len > rcvbuf->page_len)
>>
>>should be
>>
>>        if (len >= rcvbuf->page_len - sizeof(u32) || len > 1024)
>>    
>>
>
>Thanks for your feedback.  The patch to 2.4.31 that incorporates your
>suggsted changes is here:
>
>diff -u linux-2.4.31.orig/fs/nfs/nfs2xdr.c linux-2.4.31/fs/nfs/nfs2xdr.c
>--- linux-2.4.31.orig/fs/nfs/nfs2xdr.c	2002-11-28 18:53:15.000000000 -0500
>+++ linux-2.4.31/fs/nfs/nfs2xdr.c	2005-09-14 15:33:30.000000000 -0400
>@@ -571,8 +571,8 @@
> 	strlen = (u32*)kmap(rcvbuf->pages[0]);
> 	/* Convert length of symlink */
> 	len = ntohl(*strlen);
>-	if (len > rcvbuf->page_len)
>-		len = rcvbuf->page_len;
>+	if (len >= rcvbuf->page_len - sizeof(u32) || len > NFS2_MAXPATHLEN)
>+		len = rcvbuf->page_len - sizeof(u32) - 1;
> 	*strlen = len;
> 	/* NULL terminate the string we got */
> 	string = (char *)(strlen + 1);
>diff -u linux-2.4.31.orig/fs/nfs/nfs3xdr.c linux-2.4.31/fs/nfs/nfs3xdr.c
>--- linux-2.4.31.orig/fs/nfs/nfs3xdr.c	2003-11-28 13:26:21.000000000 -0500
>+++ linux-2.4.31/fs/nfs/nfs3xdr.c	2005-09-14 15:33:53.000000000 -0400
>@@ -759,8 +759,8 @@
> 	strlen = (u32*)kmap(rcvbuf->pages[0]);
> 	/* Convert length of symlink */
> 	len = ntohl(*strlen);
>-	if (len > rcvbuf->page_len)
>-		len = rcvbuf->page_len;
>+	if (len >= rcvbuf->page_len - sizeof(u32))
>+		len = rcvbuf->page_len - sizeof(u32) - 1;
> 	*strlen = len;
> 	/* NULL terminate the string we got */
> 	string = (char *)(strlen + 1);
>

One other thing -- it doesn't seem particularly correct to me to just
silently truncate the symbolic link contents.  If the contents can not
be handled correctly because they are too large, then some sort of error
should be generated.  Silently truncating could lead to interoperability
issues when multiple clients handle the contents in different fashions,
some truncating, some returning errors, and some handling the long returns.

    Thanx...

       ps

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Assar]

Peter Staubach <staubach@redhat.com> writes:
> One other thing -- it doesn't seem particularly correct to me to just
> silently truncate the symbolic link contents.

Sure, and 2.6 indeed returns ENAMETOOLONG.  I was just trying to close
the problem and not change the functionality in 2.4.  If the consensus
is that we should change it to return an error, I can certainly cook
up patches for that.

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Peter Staubach]

Assar wrote:

>Peter Staubach <staubach@redhat.com> writes:
>  
>
>>One other thing -- it doesn't seem particularly correct to me to just
>>silently truncate the symbolic link contents.
>>    
>>
>
>Sure, and 2.6 indeed returns ENAMETOOLONG.  I was just trying to close
>the problem and not change the functionality in 2.4.  If the consensus
>is that we should change it to return an error, I can certainly cook
>up patches for that.
>

Understand.  I would recommend that the 2.4 kernel be modified to return
an error, since we are already modifying the area anyway.

    Thanx...

       ps

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Assar]

Peter Staubach <staubach@redhat.com> writes:
> Understand.  I would recommend that the 2.4 kernel be modified to return
> an error, since we are already modifying the area anyway.

Marcelo: what's your thought?

If you want to go that route, the patch below should do it.

diff -u linux-2.4.31.orig/fs/nfs/nfs2xdr.c linux-2.4.31/fs/nfs/nfs2xdr.c
--- linux-2.4.31.orig/fs/nfs/nfs2xdr.c	2002-11-28 18:53:15.000000000 -0500
+++ linux-2.4.31/fs/nfs/nfs2xdr.c	2005-09-14 16:57:21.000000000 -0400
@@ -571,8 +571,11 @@
 	strlen = (u32*)kmap(rcvbuf->pages[0]);
 	/* Convert length of symlink */
 	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len)
-		len = rcvbuf->page_len;
+	if (len >= rcvbuf->page_len - sizeof(u32) || len > NFS2_MAXPATHLEN) {
+		printk(KERN_WARNING "NFS: server returned giant symlink!\n");
+		kunmap(rcvbuf->pages[0]);
+		return -ENAMETOOLONG;
+        }
 	*strlen = len;
 	/* NULL terminate the string we got */
 	string = (char *)(strlen + 1);
diff -u linux-2.4.31.orig/fs/nfs/nfs3xdr.c linux-2.4.31/fs/nfs/nfs3xdr.c
--- linux-2.4.31.orig/fs/nfs/nfs3xdr.c	2003-11-28 13:26:21.000000000 -0500
+++ linux-2.4.31/fs/nfs/nfs3xdr.c	2005-09-14 16:57:37.000000000 -0400
@@ -759,8 +759,11 @@
 	strlen = (u32*)kmap(rcvbuf->pages[0]);
 	/* Convert length of symlink */
 	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len)
-		len = rcvbuf->page_len;
+	if (len >= rcvbuf->page_len - sizeof(u32)) {
+		printk(KERN_WARNING "NFS: server returned giant symlink!\n");
+		kunmap(rcvbuf->pages[0]);
+		return -ENAMETOOLONG;
+        }
 	*strlen = len;
 	/* NULL terminate the string we got */
 	string = (char *)(strlen + 1);

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Assar]

Peter Staubach <staubach@redhat.com> writes:
> This code appears to assume that rcvbuf->page_base is zero here, but then
> uses rcvbuf->page_base when calculating where to place the null byte.  It
> seems to me that it should either use rcvbuf->page_base in both
> calculations or neither.

The meaning of page_len and page_base are not totally clear to me.  Is
it the case that the data starts at offset page_base and there is
page_len bytes of it.  If that's the case, I think the code is doing
the right thing.

Re: [PATCH] nfs client, kernel 2.4.31: readlink result overflow

[Peter Staubach]

Assar wrote:

>Peter Staubach <staubach@redhat.com> writes:
>  
>
>>This code appears to assume that rcvbuf->page_base is zero here, but then
>>uses rcvbuf->page_base when calculating where to place the null byte.  It
>>seems to me that it should either use rcvbuf->page_base in both
>>calculations or neither.
>>    
>>
>
>The meaning of page_len and page_base are not totally clear to me.  Is
>it the case that the data starts at offset page_base and there is
>page_len bytes of it.  If that's the case, I think the code is doing
>the right thing.
>

I am not clear either, but I would think that kmap_atomic() would return
a pointer to the beginning of the page.  There is also an assumption that
there is only one page.  If page_base needs to be used to offset from
the address returned by kmap_atomic(), then I wouldn't think that the
current test is correct.  I think that it needs to take page_base into
account when checking for the boundary.

    Thanx...

       ps