[RFC] nfsd: make nfs4xdr WRITEMEM safe against zero count

[RFC] nfsd: make nfs4xdr WRITEMEM safe against zero count

[Benny Halevy]

WRITEMEM zeroes the last word in the destination buffer
for padding purposes, but this must not be done if
no bytes are to be copied, as it would result
in zeroing of the word right before the array.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>

--
The current implementation works since it's always called
with non zero nbytes or it follows an encoding of the
string (or opaque) length which, if equal to zero, 
can be overwritten with zero.

Another way of implementing that is the way it's done in
fs/nfsd/nfs4callback.c:

static inline __be32 *
xdr_writemem(__be32 *p, const void *ptr, int nbytes)
{
	int tmp = XDR_QUADLEN(nbytes);
	if (!tmp)
		return p;
	p[tmp-1] = 0;
	memcpy(p, ptr, nbytes);
	return p + tmp;
}

#define WRITEMEM(ptr,nbytes)     do {                           \
	p = xdr_writemem(p, ptr, nbytes);                       \
} while (0)

Also, is there any plan to clean up the xdr encoding definition
any time and define a cleaner static inline API?
I'd be happy to take a stab at it and come up with a
written and tested proposal.

Benny

git diff --stat -p
 fs/nfsd/nfs4xdr.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index e8e27fb..588c9f6 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1712,7 +1712,7 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
 	*p++ = htonl((u32)((n) >> 32));				\
 	*p++ = htonl((u32)(n));					\
 } while (0)
-#define WRITEMEM(ptr,nbytes)     do {				\
+#define WRITEMEM(ptr,nbytes)     do if (nbytes > 0) {		\
 	*(p + XDR_QUADLEN(nbytes) -1) = 0;                      \
 	memcpy(p, ptr, nbytes);					\
 	p += XDR_QUADLEN(nbytes);				\

Re: [RFC] nfsd: make nfs4xdr WRITEMEM safe against zero count

[Chuck Lever]

On May 29, 2008, at 5:56 AM, Benny Halevy wrote:
> WRITEMEM zeroes the last word in the destination buffer
> for padding purposes, but this must not be done if
> no bytes are to be copied, as it would result
> in zeroing of the word right before the array.
>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>
> --
> The current implementation works since it's always called
> with non zero nbytes or it follows an encoding of the
> string (or opaque) length which, if equal to zero,
> can be overwritten with zero.
>
> Another way of implementing that is the way it's done in
> fs/nfsd/nfs4callback.c:
>
> static inline __be32 *
> xdr_writemem(__be32 *p, const void *ptr, int nbytes)
> {
> 	int tmp = XDR_QUADLEN(nbytes);
> 	if (!tmp)
> 		return p;
> 	p[tmp-1] = 0;
> 	memcpy(p, ptr, nbytes);
> 	return p + tmp;
> }
>
> #define WRITEMEM(ptr,nbytes)     do {                           \
> 	p = xdr_writemem(p, ptr, nbytes);                       \
> } while (0)
>
> Also, is there any plan to clean up the xdr encoding definition
> any time and define a cleaner static inline API?
> I'd be happy to take a stab at it and come up with a
> written and tested proposal.

I took a long jump at this last year, but no-one else seemed  
interested in the work.

> git diff --stat -p
> fs/nfsd/nfs4xdr.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e8e27fb..588c9f6 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1712,7 +1712,7 @@ nfsd4_decode_compound(struct  
> nfsd4_compoundargs *argp)
> 	*p++ = htonl((u32)((n) >> 32));				\
> 	*p++ = htonl((u32)(n));					\
> } while (0)
> -#define WRITEMEM(ptr,nbytes)     do {				\
> +#define WRITEMEM(ptr,nbytes)     do if (nbytes > 0) {		\
> 	*(p + XDR_QUADLEN(nbytes) -1) = 0;                      \
> 	memcpy(p, ptr, nbytes);					\
> 	p += XDR_QUADLEN(nbytes);				\
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"  
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC] nfsd: make nfs4xdr WRITEMEM safe against zero count

[Chuck Lever]

On May 29, 2008, at 5:56 AM, Benny Halevy wrote:
> WRITEMEM zeroes the last word in the destination buffer
> for padding purposes, but this must not be done if
> no bytes are to be copied, as it would result
> in zeroing of the word right before the array.
>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>
> --
> The current implementation works since it's always called
> with non zero nbytes or it follows an encoding of the
> string (or opaque) length which, if equal to zero,
> can be overwritten with zero.
>
> Another way of implementing that is the way it's done in
> fs/nfsd/nfs4callback.c:
>
> static inline __be32 *
> xdr_writemem(__be32 *p, const void *ptr, int nbytes)
> {
> 	int tmp = XDR_QUADLEN(nbytes);
> 	if (!tmp)
> 		return p;
> 	p[tmp-1] = 0;
> 	memcpy(p, ptr, nbytes);
> 	return p + tmp;
> }
>
> #define WRITEMEM(ptr,nbytes)     do {                           \
> 	p = xdr_writemem(p, ptr, nbytes);                       \
> } while (0)
>
> Also, is there any plan to clean up the xdr encoding definition
> any time and define a cleaner static inline API?
> I'd be happy to take a stab at it and come up with a
> written and tested proposal.

By the way, here is a pointer to the old work:

   http://git.linux-nfs.org/?p=cel/ 
cel-2.6.git;a=shortlog;h=drop-20070423

The work in this snapshot is only partially complete.

There are many many issues to fix up.  One of the bigger ones is that  
the RPC layer still takes the BKL during XDR encoding and decoding.   
But I wanted to create something that could be fully or partially  
shared by the client and server implementations, and that would be  
more rigorous about static and dynamic type checking.

In addition, the NFSv4 routines use a new style of managing the RPC  
buffer that I wanted to adopt for the other in-kernel XDR routines,  
then convert the RPC client to use only the new style when invoking  
XDR routines.  I never got that far, but I still think it's a  
reasonable goal.

> git diff --stat -p
> fs/nfsd/nfs4xdr.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e8e27fb..588c9f6 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1712,7 +1712,7 @@ nfsd4_decode_compound(struct  
> nfsd4_compoundargs *argp)
> 	*p++ = htonl((u32)((n) >> 32));				\
> 	*p++ = htonl((u32)(n));					\
> } while (0)
> -#define WRITEMEM(ptr,nbytes)     do {				\
> +#define WRITEMEM(ptr,nbytes)     do if (nbytes > 0) {		\
> 	*(p + XDR_QUADLEN(nbytes) -1) = 0;                      \
> 	memcpy(p, ptr, nbytes);					\
> 	p += XDR_QUADLEN(nbytes);				\
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"  
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC] nfsd: make nfs4xdr WRITEMEM safe against zero count

[J. Bruce Fields]

On Thu, May 29, 2008 at 12:24:58PM -0400, Chuck Lever wrote:
> By the way, here is a pointer to the old work:
>
>   http://git.linux-nfs.org/?p=cel/cel-2.6.git;a=shortlog;h=drop-20070423
>
> The work in this snapshot is only partially complete.
>
> There are many many issues to fix up.  One of the bigger ones is that  
> the RPC layer still takes the BKL during XDR encoding and decoding.  But 
> I wanted to create something that could be fully or partially shared by 
> the client and server implementations, and that would be more rigorous 
> about static and dynamic type checking.
>
> In addition, the NFSv4 routines use a new style of managing the RPC  
> buffer that I wanted to adopt for the other in-kernel XDR routines, then 
> convert the RPC client to use only the new style when invoking XDR 
> routines.  I never got that far, but I still think it's a reasonable 
> goal.

To find something manageable to start with, what's the minimum first
step that that would achieve Benny's goal of replacing those server-side
xdr macros?

>From a quick glance at the patches: a kerneldoc comment for each xdr
encode/decode seems like overkill, since it's mostly boilerplate; could
we document those arguments just once with the definition of kxdrproc_t,
then refer to that comment as necessary?

Anyway, seems like good stuff to me.

--b.

Re: [RFC] nfsd: make nfs4xdr WRITEMEM safe against zero count

[Chuck Lever]

On Jun 2, 2008, at 12:49 PM, J. Bruce Fields wrote:
> On Thu, May 29, 2008 at 12:24:58PM -0400, Chuck Lever wrote:
>> By the way, here is a pointer to the old work:
>>
>>  http://git.linux-nfs.org/?p=cel/cel-2.6.git;a=shortlog;h=drop-20070423
>>
>> The work in this snapshot is only partially complete.
>>
>> There are many many issues to fix up.  One of the bigger ones is that
>> the RPC layer still takes the BKL during XDR encoding and  
>> decoding.  But
>> I wanted to create something that could be fully or partially  
>> shared by
>> the client and server implementations, and that would be more  
>> rigorous
>> about static and dynamic type checking.
>>
>> In addition, the NFSv4 routines use a new style of managing the RPC
>> buffer that I wanted to adopt for the other in-kernel XDR routines,  
>> then
>> convert the RPC client to use only the new style when invoking XDR
>> routines.  I never got that far, but I still think it's a reasonable
>> goal.
>
> To find something manageable to start with, what's the minimum first
> step that that would achieve Benny's goal of replacing those server- 
> side
> xdr macros?

If we're going to do this, I think we want to replace the macros in  
both the server and the client.

Thus you want to have a set of shared static inline functions that  
manage static type checking, data conversion, and bounds checking for  
basic XDR data types.  There are a short series of patches near the  
beginning of that branch that introduce these helpers.  Look at  
06e537f7 through cf1ba5ab.

The last one adds generic warning messages on XDR encode and decode  
errors.  We can certainly remove (or provide a kernel config switch  
for) the decode warnings as they could cause a DoS, but the encode  
error messages might be useful, and would allow us to remove many of  
the ad hoc printk's from the XDR functions themselves, resulting in  
smaller code.

Then, 986ac05e defines the set of NFSv4 XDR data types laid out in  
RFC3530.  The following patches start to fix up the client-side  
encoders; I didn't get to the client-side decoders, or to the server.

The real effort is going to be reworking the encoders and decoders for  
the big NFSv4 datatypes, like for instance the arguments and results  
that contain attributes.

> From a quick glance at the patches: a kerneldoc comment for each xdr
> encode/decode seems like overkill, since it's mostly boilerplate;  
> could
> we document those arguments just once with the definition of  
> kxdrproc_t,
> then refer to that comment as necessary?

A lot of the changes in that drop are excessive; it was a prototype,  
so I used the comments to keep track of where I was heading.  However,  
in general I think it's important to document the assumptions relating  
to the arguments, and the possible return values, for each public  
function.

Many of the comments in these patches also copy the definition of the  
XDR data type from the RFC, and call out the RFC section where it is  
specified.  This provides a clear connection between the  
implementation and the spec to allow easier code review and maintenance.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC] nfsd: make nfs4xdr WRITEMEM safe against zero count

[J. Bruce Fields]

On Thu, May 29, 2008 at 12:56:08PM +0300, Benny Halevy wrote:
> WRITEMEM zeroes the last word in the destination buffer
> for padding purposes, but this must not be done if
> no bytes are to be copied, as it would result
> in zeroing of the word right before the array.

Thanks, applied.

> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> 
> --
> The current implementation works since it's always called
> with non zero nbytes or it follows an encoding of the
> string (or opaque) length which, if equal to zero, 
> can be overwritten with zero.

I also added the above paragraph to the changelog.

> 
> Another way of implementing that is the way it's done in
> fs/nfsd/nfs4callback.c:
> 
> static inline __be32 *
> xdr_writemem(__be32 *p, const void *ptr, int nbytes)
> {
> 	int tmp = XDR_QUADLEN(nbytes);
> 	if (!tmp)
> 		return p;
> 	p[tmp-1] = 0;
> 	memcpy(p, ptr, nbytes);
> 	return p + tmp;
> }
> 
> #define WRITEMEM(ptr,nbytes)     do {                           \
> 	p = xdr_writemem(p, ptr, nbytes);                       \
> } while (0)
> 
> Also, is there any plan to clean up the xdr encoding definition
> any time and define a cleaner static inline API?

Not currently.

> I'd be happy to take a stab at it and come up with a
> written and tested proposal.

That could be helpful, thanks.

--b.

> 
> Benny
> 
> git diff --stat -p
>  fs/nfsd/nfs4xdr.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e8e27fb..588c9f6 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1712,7 +1712,7 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
>  	*p++ = htonl((u32)((n) >> 32));				\
>  	*p++ = htonl((u32)(n));					\
>  } while (0)
> -#define WRITEMEM(ptr,nbytes)     do {				\
> +#define WRITEMEM(ptr,nbytes)     do if (nbytes > 0) {		\
>  	*(p + XDR_QUADLEN(nbytes) -1) = 0;                      \
>  	memcpy(p, ptr, nbytes);					\
>  	p += XDR_QUADLEN(nbytes);				\
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html