Re: fuzz tested user mode linux crashed in NFS code path

["J. Bruce Fields" <bfields@fieldses.org>]

On Wed, Jul 23, 2014 at 01:04:35PM +0800, Kinglong Mee wrote:
> On 7/21/2014 23:55, J. Bruce Fields wrote:
> > On Sat, Jul 19, 2014 at 11:23:59AM +0800, Kinglong Mee wrote:
> >> On Sat, Jul 19, 2014 at 12:50 AM, Toralf Förster <toralf.foerster@gmx.de> wrote:
> >>> On 07/18/2014 06:22 PM, Toralf Förster wrote:
> >>>> I can now try with kzalloc, but due to the nature of this issue I think, that the absence of this crash - even after 2-3 hours - doesn't mean by 100%, that kzalloc fixed it, or ?
> >>>
> >>> Well, next crash (with kzalloc patch) happened after 20 minutes ...
> >>
> >> Maybe I have found the problem.
> >> The stateid and denied are defined as an union as,
> >> fs/nfsd/xdr4.h
> >> 145 struct nfsd4_lock_denied {
> >> 146         clientid_t      ld_clientid;
> >> 147         struct xdr_netobj       ld_owner;
> >> 148         u64             ld_start;
> >> 149         u64             ld_length;
> >> 150         u32             ld_type;
> >> 151 };
> >> 152
> >> 153 struct nfsd4_lock {
> >> ... ...
> >> 174         /* response */
> >> 175         union {
> >> 176                 struct {
> >> 177                         stateid_t               stateid;
> >> 178                 } ok;
> >> 179                 struct nfsd4_lock_denied        denied;
> >> 180         } u;
> >>
> >>  30 struct xdr_netobj {
> >>  31         unsigned int            len;
> >>  32         u8 *                    data;
> >>  33 };
> >>
> >> sizeof(stateid_t) = 16, sizeof(clientid_t) = 8,
> >> sizeof(struct xdr_netobj) = 16, (on x86_x64 platform),
> >> sizeof(struct xdr_netobj) = 8,  (on i686 platform)
> >>
> >> Lock file success, nfsd will copy stateid to the union, but the value
> >> also influence denied.
> >> If on x86_64 platform, only influence the len in xdr_netobj,
> >> but on i686 platform, will influence the len and the data in xdr_netobj.
> >> So, the problem only appears on i686 platform.
> > 
> > Oh, great catch, thanks.  Sounds like that would explain all of Toralf's
> > results.
> > 
> > I'll include this explanation with your original patch and submit it for
> > 3.16.
> 
> I saw the patch in your git-tree, there is one fault about the comments.
> 
> > (Note that lock->lk_denied.ld_owner.data appears it should be zero here,
> > until you notice that it's one arm of a union the other arm of which is
> > written to in the succesful case by the
> > 
> > memcpy(&oc->oc_resp_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
> > 
> > in nfsd4_open_confirm(). 
> 
> It's, 
>  memcpy(&lock->lk_resp_stateid, &lock_stp->st_stid.sc_stateid,
>                                 sizeof(stateid_t));
> in nfsd4_lock().

Thanks, fixed!

--b.