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

[Peter Staubach <staubach@redhat.com>]

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