Date: Tue, 7 May 2019 18:07:01 -0700 From: Conrad Meyer <cem@freebsd.org> To: Kirk McKusick <mckusick@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r347066 - in head: sbin/fsck_ffs sys/ufs/ufs Message-ID: <CAG6CVpWhXpjoZPxO4BSCQAcLfgNeqXj_eDOaoo_06HUcAo0krg@mail.gmail.com> In-Reply-To: <201905032154.x43LsFae008760@repo.freebsd.org> References: <201905032154.x43LsFae008760@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Kirk, Coverity points out that namlen may be used uninitialized in the following sequence (CID 1401317): On Fri, May 3, 2019 at 2:54 PM Kirk McKusick <mckusick@freebsd.org> wrote: > > Author: mckusick > Date: Fri May 3 21:54:14 2019 > New Revision: 347066 > URL: https://svnweb.freebsd.org/changeset/base/347066 > > Log: > This update eliminates a kernel stack disclosure bug in UFS/FFS > directory entries that is caused by uninitialized directory entry > padding written to the disk. > ... > --- head/sbin/fsck_ffs/dir.c Fri May 3 21:48:42 2019 (r347065) > +++ head/sbin/fsck_ffs/dir.c Fri May 3 21:54:14 2019 (r347066) > ... > @@ -209,15 +230,39 @@ dircheck(struct inodesc *idesc, struct direct *dp) > char *cp; > u_char type; > u_int8_t namlen; > - int spaceleft; > + int spaceleft, modified, unused; > > + modified = 0; > spaceleft = DIRBLKSIZ - (idesc->id_loc % DIRBLKSIZ); > if (dp->d_reclen == 0 || > dp->d_reclen > spaceleft || > - (dp->d_reclen & 0x3) != 0) > + (dp->d_reclen & (DIR_ROUNDUP - 1)) != 0) > goto bad; > - if (dp->d_ino == 0) > - return (1); > + if (dp->d_ino == 0) { In this case, namlen may never be initialized. > + /* > + * Special case of an unused directory entry. Normally > + * the kernel would coalesce unused space with the previous > + * entry by extending its d_reclen, but there are situations > + * (e.g. fsck) where that doesn't occur. > + * If we're clearing out directory cruft (-z flag), then make > + * sure this entry gets fully cleared as well. > + */ > + if (zflag && fswritefd >= 0) { > + if (dp->d_type != 0) { > + dp->d_type = 0; > + modified = 1; > + } > + if (dp->d_namlen != 0) { > + dp->d_namlen = 0; > + modified = 1; > + } > + if (dp->d_name[0] != '\0') { > + dp->d_name[0] = '\0'; > + modified = 1; > + } > + } > + goto good; Then we jump 'good'. > + } > size = DIRSIZ(0, dp); > namlen = dp->d_namlen; > type = dp->d_type; > @@ -231,7 +276,37 @@ dircheck(struct inodesc *idesc, struct direct *dp) > goto bad; > if (*cp != '\0') > goto bad; > + > +good: > + if (zflag && fswritefd >= 0) { > + /* > + * Clear unused directory entry space, including the d_name > + * padding. > + */ > + /* First figure the number of pad bytes. */ > + unused = roundup2(namlen + 1, DIR_ROUNDUP) - (namlen + 1); And here we access uninitialized 'namlen'. Best, Conrad
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpWhXpjoZPxO4BSCQAcLfgNeqXj_eDOaoo_06HUcAo0krg>