From owner-freebsd-current@FreeBSD.ORG Tue Nov 18 17:05:24 2003 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id CA36E16A4CE for ; Tue, 18 Nov 2003 17:05:24 -0800 (PST) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2F8D143FAF for ; Tue, 18 Nov 2003 17:05:23 -0800 (PST) (envelope-from bde@zeta.org.au) Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3p2/8.8.7) with ESMTP id MAA08070; Wed, 19 Nov 2003 12:04:59 +1100 Date: Wed, 19 Nov 2003 12:04:58 +1100 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Rudolf Cejka In-Reply-To: <20031118164618.GA84637@fit.vutbr.cz> Message-ID: <20031119112001.P5091@gamplex.bde.org> References: <200311142119.hAELJPaG011962@beastie.mckusick.com> <20031118164618.GA84637@fit.vutbr.cz> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: freebsd-current@freebsd.org cc: Kirk McKusick Subject: Re: HEADS-UP new statfs structure X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Nov 2003 01:05:24 -0000 X-List-Received-Date: Wed, 19 Nov 2003 01:05:24 -0000 X-List-Received-Date: Wed, 19 Nov 2003 01:05:24 -0000 X-List-Received-Date: Wed, 19 Nov 2003 01:05:24 -0000 On Tue, 18 Nov 2003, Rudolf Cejka wrote: > Hello, and is it possible to review some my (one's from masses :o) > questions/suggestions? > > * cvtstatfs() for freebsd4_* compat syscalls does not copy text fields > correctly, so old binaries with new kernel know just about first > 16 characters from mount points - what do you think about the > following patch? (Or maybe with even safer sizeof() - but I did not > test it.) Hmm, there were 2 bugs here: - MFSNAMELEN was confused with MNAMELEN in some places. This gives unterminated strings as well as excessively truncated strings. - there were off-by-1 errors which would have given unterminated strings even without the previous bug. > --- sys/kern/vfs_syscalls.c.orig Sun Nov 16 11:12:09 2003 > +++ sys/kern/vfs_syscalls.c Sun Nov 16 11:56:07 2003 > @@ -645,11 +645,11 @@ > osp->f_syncreads = MIN(nsp->f_syncreads, LONG_MAX); > osp->f_asyncreads = MIN(nsp->f_asyncreads, LONG_MAX); > bcopy(nsp->f_fstypename, osp->f_fstypename, > - MIN(MFSNAMELEN, OMNAMELEN)); > + MIN(MFSNAMELEN, OMFSNAMELEN - 1)); MFSNAMELEN didn't change, so there is currently only a logical problem here. The -1 term could be moved outside of the MIN(). It works in either place and would save duplicating the terminating NUL in the unlikely event that the new name length becomes smaller than the old one. I'm not sure which is clearest. > bcopy(nsp->f_mntonname, osp->f_mntonname, > - MIN(MFSNAMELEN, OMNAMELEN)); > + MIN(MNAMELEN, OMNAMELEN - 1)); Similarly, plus the larger bug. MNAMELEN increased from (88 - 2 * sizeof(long)) to 88, so if it were used without the -1 in the above, then mount point name lengths longer than the old value would have been unterminated instead of truncated. > bcopy(nsp->f_mntfromname, osp->f_mntfromname, > - MIN(MFSNAMELEN, OMNAMELEN)); > + MIN(MNAMELEN, OMNAMELEN - 1)); Similarly. > if (suser(td)) { > osp->f_fsid.val[0] = osp->f_fsid.val[1] = 0; > } else { > --- > > * sys/compat/freebsd32/freebsd32_misc.c: If you look into copy_statfs(), > you copy 88-byte strings into just 80-byte strings. Fortunately it seems > that there are just overwritten spare fields and f_syncreads/f_asyncreads > before they are set to the correct value. What about these patches, which > furthermore are resistant to possible MFSNAMELEN change in the future? > [I'm sorry, these patches are untested] > > --- sys/compat/freebsd32/freebsd32.h.orig Tue Nov 18 16:58:28 2003 > +++ sys/compat/freebsd32/freebsd32.h Tue Nov 18 16:59:36 2003 > @@ -75,6 +75,7 @@ > int32_t ru_nivcsw; > }; > > +#define FREEBSD32_MFSNAMELEN 16 /* length of type name including null */ > #define FREEBSD32_MNAMELEN (88 - 2 * sizeof(int32_t)) /* size of on/from name bufs */ > MFSNAMELEN hasn't changed, so this part is cosmetic. But don't we now need to clone all of this compatibility cruft for the new statfs()? Native 32-bit systems have both. Then MFSNAMELEN for this version should probably be spelled OMFSNAMELEN. > struct statfs32 { > @@ -92,7 +93,7 @@ > int32_t f_flags; > int32_t f_syncwrites; > int32_t f_asyncwrites; > - char f_fstypename[MFSNAMELEN]; > + char f_fstypename[FREEBSD32_MFSNAMELEN]; > char f_mntonname[FREEBSD32_MNAMELEN]; > int32_t f_syncreads; > int32_t f_asyncreads; > --- sys/compat/freebsd32/freebsd32_misc.c.orig Tue Nov 18 16:59:49 2003 > +++ sys/compat/freebsd32/freebsd32_misc.c Tue Nov 18 17:03:31 2003 > @@ -276,6 +276,7 @@ > static void > copy_statfs(struct statfs *in, struct statfs32 *out) > { > + bzero(out, sizeof *out); Yikes. All copied out structs that might have holes (i.e., all structs unless you want to examine them in binary for every combination of arch/compiler/etc) need to be bzero()ed like this, but there are no bzero()'s in files in this directory. > CP(*in, *out, f_bsize); > CP(*in, *out, f_iosize); > CP(*in, *out, f_blocks); > @@ -290,14 +291,14 @@ > CP(*in, *out, f_flags); > CP(*in, *out, f_syncwrites); > CP(*in, *out, f_asyncwrites); > - bcopy(in->f_fstypename, > - out->f_fstypename, MFSNAMELEN); > - bcopy(in->f_mntonname, > - out->f_mntonname, MNAMELEN); > + bcopy(in->f_fstypename, out->f_fstypename, > + MIN(MFSNAMELEN, FREEBSD32_MFSNAMELEN - 1)); > + bcopy(in->f_mntonname, out->f_mntonname, > + MIN(MNAMELEN, FREEBSD32_MNAMELEN - 1)); > CP(*in, *out, f_syncreads); > CP(*in, *out, f_asyncreads); > - bcopy(in->f_mntfromname, > - out->f_mntfromname, MNAMELEN); > + bcopy(in->f_mntfromname, out->f_mntfromname, > + MIN(MNAMELEN, FREEBSD32_MNAMELEN - 1)); > } > > int This seems to be correct except possibly for the style (placement of -1 and fixing the indentation of the continuation lines so that it is not bug-for-bug compatible with the rest of the file). > --- > > * sys/ia64/ia32/ia32.h: It seems to me that statfs32 has similar problems > as freebsd32.h - however in this case real and bigger, because statfs32 > is now a combination of old and new statfs: old 32bit fields with new > string lengths, so sizeof(statfs32) is changed from 256 to 272. Is this > really correct? If I understand it correctly, it breaks binary > compatibility for old ia32 binaries. I think that MFSNAMELEN/MNAMELEN > should be renamed to OMFSNAMELEN/OMNAMELEN, or ia32.h should define own > IA32_MFSNAMELEN/IA32_MNAMELEN, similarly to freebsd32.h - which is more > secure with respect to further updates in the future. > ... Yikes :-). Bruce