Patchwork [BUG:2972] NFS : Handle buffer overflow in nfs3_create_exclusive.

login
register
Submitter Gaurav
Date 2011-06-01 05:10:27
Message ID <1306905027-596-1-git-send-email-gaurav@gluster.com>
Download mbox | patch
Permalink /patch/7344/
State Accepted
Headers show

Comments

Gaurav - 2011-06-01 05:10:27
From: Gaurav <gaurav@gluster.com>


Signed-off-by: Gaurav <gaurav@gluster.com>
---
 xlators/nfs/server/src/nfs3.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Anand Avati - 2011-06-01 05:37:40
> -        cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000);
> -        cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF);
> +        memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof (cs->stbuf.ia_atime));
> +        memcpy (&cs->stbuf.ia_mtime,
> +                ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime),
> +                sizeof (cs->stbuf.ia_mtime));

atime is expected to be the 32 most significant bits and mtime the 32
least significant bits. The diff assumes the architecture to be
big-endian. Please use bitmasking + shifting and typecasting to fix
the overflow.

Avati
Shehjar Tikoo - 2011-06-01 06:06:49
Anand Avati wrote:
>> -        cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000);
>> -        cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF);
>> +        memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof (cs->stbuf.ia_atime));
>> +        memcpy (&cs->stbuf.ia_mtime,
>> +                ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime),
>> +                sizeof (cs->stbuf.ia_mtime));
> 
> atime is expected to be the 32 most significant bits and mtime the 32
> least significant bits. The diff assumes the architecture to be
> big-endian. Please use bitmasking + shifting and typecasting to fix
> the overflow.

Thats fine, we're not interested in the order when storing in atime and 
mtime, cookieverf is going to be an opaque value from the client which we 
want to store "somewhere", atime and mtime were conveniently available in 
this fop. The value does not have to be intelligible, only recoverable 
through atime and mtime when a stat is performed at a later time.

Gaurav, if you're not going to CC me, you'll just delay review of these 
patches.



> 
> Avati
> _______________________________________________
> glusterfs mailing list
> glusterfs@dev.gluster.com
> http://dev.gluster.com/mailman/listinfo/glusterfs
Gaurav - 2011-06-01 06:18:39
There is no such requirement of storing most significant 32 bits in atime and least significant 32 bits in mtime,
at the time of varification in "nfs3svc_create_stat_cbk" they are compared as two 32 bit values instead of single 64 bit value, so it should work irrespective
of endianess.

Regards,
Gaurav

----- Original Message -----
> From: "Anand Avati" <avati@gluster.com>
> To: gaurav@gluster.com
> Cc: glusterfs@dev.gluster.com, avati@dev.gluster.com
> Sent: Wednesday, June 1, 2011 11:07:40 AM
> Subject: Re: [PATCH BUG:2972] NFS : Handle buffer overflow in nfs3_create_exclusive.
> > - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000);
> > - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF);
> > + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof
> > (cs->stbuf.ia_atime));
> > + memcpy (&cs->stbuf.ia_mtime,
> > + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime),
> > + sizeof (cs->stbuf.ia_mtime));
> 
> atime is expected to be the 32 most significant bits and mtime the 32
> least significant bits. The diff assumes the architecture to be
> big-endian. Please use bitmasking + shifting and typecasting to fix
> the overflow.
> 
> Avati
Anand Avati - 2011-06-01 06:20:22
Do you mean that those two "pieces" of numbers end up being the atime
and mtime of the file on disk? Can you throw some light on this? What
is cookieverf?

Avati

On Wed, Jun 1, 2011 at 11:48 AM, Gangalwar <gaurav@gluster.com> wrote:
> There is no such requirement of storing most significant 32 bits in atime and least significant 32 bits in mtime,
> at the time of varification in "nfs3svc_create_stat_cbk" they are compared as two 32 bit values instead of single 64 bit value, so it should work irrespective
> of endianess.
>
> Regards,
> Gaurav
>
> ----- Original Message -----
>> From: "Anand Avati" <avati@gluster.com>
>> To: gaurav@gluster.com
>> Cc: glusterfs@dev.gluster.com, avati@dev.gluster.com
>> Sent: Wednesday, June 1, 2011 11:07:40 AM
>> Subject: Re: [PATCH BUG:2972] NFS : Handle buffer overflow in nfs3_create_exclusive.
>> > - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000);
>> > - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF);
>> > + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof
>> > (cs->stbuf.ia_atime));
>> > + memcpy (&cs->stbuf.ia_mtime,
>> > + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime),
>> > + sizeof (cs->stbuf.ia_mtime));
>>
>> atime is expected to be the 32 most significant bits and mtime the 32
>> least significant bits. The diff assumes the architecture to be
>> big-endian. Please use bitmasking + shifting and typecasting to fix
>> the overflow.
>>
>> Avati
>
Gaurav - 2011-06-01 06:30:36
Coockieverf is just a 64 bit data, which we need to store in persistent storage in case of create request in exclusive mode.
We use this information to identify duplicate create request in case of retransmission from the same client, client is expected to send same coockieverf in case of retransmission.
RFC recommends to store this value in atime and mtime, it has nothing to do with actual atime and mtime values, client is expected to do setattr after successful exclusive create, so these values will be overwritten.

Regards,
Gaurav

----- Original Message -----
> From: "Anand Avati" <avati@gluster.com>
> To: "Gangalwar" <gaurav@gluster.com>
> Cc: glusterfs@dev.gluster.com, avati@dev.gluster.com
> Sent: Wednesday, June 1, 2011 11:50:22 AM
> Subject: Re: [PATCH BUG:2972] NFS : Handle buffer overflow in nfs3_create_exclusive.
> Do you mean that those two "pieces" of numbers end up being the atime
> and mtime of the file on disk? Can you throw some light on this? What
> is cookieverf?
> 
> Avati
> 
> On Wed, Jun 1, 2011 at 11:48 AM, Gangalwar <gaurav@gluster.com> wrote:
> > There is no such requirement of storing most significant 32 bits in
> > atime and least significant 32 bits in mtime,
> > at the time of varification in "nfs3svc_create_stat_cbk" they are
> > compared as two 32 bit values instead of single 64 bit value, so it
> > should work irrespective
> > of endianess.
> >
> > Regards,
> > Gaurav
> >
> > ----- Original Message -----
> >> From: "Anand Avati" <avati@gluster.com>
> >> To: gaurav@gluster.com
> >> Cc: glusterfs@dev.gluster.com, avati@dev.gluster.com
> >> Sent: Wednesday, June 1, 2011 11:07:40 AM
> >> Subject: Re: [PATCH BUG:2972] NFS : Handle buffer overflow in
> >> nfs3_create_exclusive.
> >> > - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000);
> >> > - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF);
> >> > + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof
> >> > (cs->stbuf.ia_atime));
> >> > + memcpy (&cs->stbuf.ia_mtime,
> >> > + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime),
> >> > + sizeof (cs->stbuf.ia_mtime));
> >>
> >> atime is expected to be the 32 most significant bits and mtime the
> >> 32
> >> least significant bits. The diff assumes the architecture to be
> >> big-endian. Please use bitmasking + shifting and typecasting to fix
> >> the overflow.
> >>
> >> Avati
> >
Gaurav - 2011-06-01 06:36:23
Right Shehjar, will keep that in mind. 

Regards,
Gaurav

----- Original Message -----
> From: "Shehjar Tikoo" <shehjart@gluster.com>
> To: "Anand Avati" <avati@gluster.com>
> Cc: gaurav@gluster.com, glusterfs@dev.gluster.com, avati@dev.gluster.com
> Sent: Wednesday, June 1, 2011 11:36:49 AM
> Subject: Re: [PATCH BUG:2972] NFS : Handle buffer overflow in nfs3_create_exclusive.
> Anand Avati wrote:
> >> - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000);
> >> - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF);
> >> + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof
> >> (cs->stbuf.ia_atime));
> >> + memcpy (&cs->stbuf.ia_mtime,
> >> + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime),
> >> + sizeof (cs->stbuf.ia_mtime));
> >
> > atime is expected to be the 32 most significant bits and mtime the
> > 32
> > least significant bits. The diff assumes the architecture to be
> > big-endian. Please use bitmasking + shifting and typecasting to fix
> > the overflow.
> 
> Thats fine, we're not interested in the order when storing in atime
> and
> mtime, cookieverf is going to be an opaque value from the client which
> we
> want to store "somewhere", atime and mtime were conveniently available
> in
> this fop. The value does not have to be intelligible, only recoverable
> through atime and mtime when a stat is performed at a later time.
> 
> Gaurav, if you're not going to CC me, you'll just delay review of
> these
> patches.
> 
> 
> 
> >
> > Avati
> > _______________________________________________
> > glusterfs mailing list
> > glusterfs@dev.gluster.com
> > http://dev.gluster.com/mailman/listinfo/glusterfs

Patch

diff --git a/xlators/nfs/server/src/nfs3.c b/xlators/nfs/server/src/nfs3.c
index 13eb1c0..2fc295e 100644
--- a/xlators/nfs/server/src/nfs3.c
+++ b/xlators/nfs/server/src/nfs3.c
@@ -2437,8 +2437,10 @@  nfs3_create_exclusive (nfs3_call_state_t *cs)
 
         /* Storing verifier as a mtime and atime attribute, to store it
          * in stable storage */
-        cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000);
-        cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF);
+        memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof (cs->stbuf.ia_atime));
+        memcpy (&cs->stbuf.ia_mtime,
+                ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime),
+                sizeof (cs->stbuf.ia_mtime));
         cs->setattr_valid |= GF_SET_ATTR_ATIME;
         cs->setattr_valid |= GF_SET_ATTR_MTIME;
         nfs_request_user_init (&nfu, cs->req);