Date: Mon, 15 Jan 2007 10:59:21 +0100 From: Alexander Leidinger <Alexander@Leidinger.net> To: Scot Hetzel <swhetzel@gmail.com> Cc: freebsd-emulation@freebsd.org Subject: Re: linuxolator: proc/filesystems and sysfs function implementations Message-ID: <20070115105921.wbv2yrd4bkgokcko@webmail.leidinger.net> In-Reply-To: <790a9fff0701141254s2c92b10ag6b042a019bc283c@mail.gmail.com> References: <790a9fff0701060012x40063f48pc842510b082df5a5@mail.gmail.com> <20070106130830.3c2e6d98@Magellan.Leidinger.net> <790a9fff0701132017g6c081567la4a759cea4618535@mail.gmail.com> <20070114105239.GA6955@stud.fit.vutbr.cz> <790a9fff0701141254s2c92b10ag6b042a019bc283c@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Quoting Scot Hetzel <swhetzel@gmail.com> (from Sun, 14 Jan 2007 =20 14:54:28 -0600): > On 1/14/07, Divacky Roman <xdivac02@stud.fit.vutbr.cz> wrote: >> + buf =3D (char *)(uintptr_t)args->arg2; > : >> + bsd_to_linux_fs(vfsp->vfc_name, &fs); >> + len =3D strlen(fs.name) + 1; >> + error =3D copyout(fs.name, buf, len); >> >> no need to check if it fits in the userspace buffer? also you >> dont check return value >> > > According to the linux man page, there isn't a way to check the size > of buf, as we are only passed the value of the pointer in args->arg2. Ugh... there's not even an implicit size because of a fixed definition =20 of the target buffer in some header? That would be very bad design. =20 The kernel could overwrite userland data even if the kernel is not =20 malicious (load a new FS with a too long name and BOOOOM). > Should I replace buf with (char *)(uintptr_t)args->arg2 in the > copyout, since it is only used there. > > http://www.die.net/doc/linux/man/man2/sysfs.2.html ---snip--- BUGS There is no libc or glibc support. There is no way to guess how large =20 buf should be. ---snip--- That's bad. That's very very bad. I don't like this in the FreeBSD =20 kernel, I want an upper bound. Would you please try to find some =20 artificial upper bound? The MFSNAMELEN would be one candidate. A =20 better candidate would be a similar Linux value. > I didn't check the return value, as error is going to be set to either > 0 or EFAULT by the copyout function. Since it's the end of the case, > it falls thru to the return statement at the bottom of the function. > >> + break; >> + >> + /* >> + * Return the total number of file system types =20 >> currently present >> + * in the kernel. >> + */ >> + case 3: >> + TAILQ_FOREACH(vfsp, &vfsconf, vfc_list) >> + index++; >> + td->td_retval[0] =3D index; >> >> does this make sense? shouldnt we return just the number of filesystems >> that we share with linux? >> > > There are only 7 filesystems that are freebsd specific (devfs, > fdescfs, mfs, nullfs, portalfs, procfs (bsd), unionfs). If we exclude > these filesystems, then we would need to ensure that we skip them in > the other two options. > > The way that it is currently emulated, is that all freebsd filesystems > (kldloaded filesystems) are available to the linuxolator. I suggest to keep the FreeBSD ones visible, we can revisit this =20 decision in case we find a case where whe shouldn't do this. >> also... you strcpy string to another string, are you sure it always =20 >> fits? how do you >> asure that? >> > > name can't be larger than MFSNAMELEN, and sizeof(fs->name) =3D=3D MFSNAMEL= EN. Please use strlcpy with the sizeof (defensive programming). >> RCS file: /home/ncvs/src/sys/compat/linux/linux_util.h,v >> retrieving revision 1.28 >> diff -u -r1.28 linux_util.h >> --- compat/linux/linux_util.h 27 Jun 2006 18:30:49 -0000 1.28 >> +++ compat/linux/linux_util.h 14 Jan 2007 02:08:42 -0000 >> @@ -101,4 +101,12 @@ >> char *linux_get_char_devices(void); >> void linux_free_get_char_devices(char *string); >> >> +struct linux_file_system_type { >> + char name[16]; >> + int fs_flags; >> +}; >> >> 16? why 16? please comment or something >> > 16 is the value of MFSNAMELEN and used by the vfsconf struct to define > the max size of vfc_name. I kept name (in the linux_file_system_type Is there a similar value in Linux? It would be better to use this if =20 it exists. > struct) the same size, so that we didn't have to worry about the size > when using strcpy. When you find a value like MFSNAMELEN on Linux, please have a look at =20 linux_misc.c:linux_prctl(), specially the LINUX_PR_{SET|GET}_NAME case =20 and the comments in there. What you are doing here should be done =20 similar. > I also wasn't sure if I should add: > > #include <sys/mount.h> > > in linux_util.h so that MFSNAMELEN could be used to define the size of > name in the struct (it may also affect other files, that include > linux_util.h). I agree with Roman here. Do you intend to submit more patches to the linuxulator? If yes, do =20 you want to get write access to our perforce repository (that's not an =20 official CVS or developer account, but a step in this direction)? If =20 yes, what username do you prefer? Bye, Alexander. --=20 I'm still waiting for the advent of the computer science groupie. http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID =3D B0063FE7 http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID =3D 72077137
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070115105921.wbv2yrd4bkgokcko>