linuxolator: proc/filesystems and sysfs function implementations

[Alexander Leidinger]

Quoting Scot Hetzel <swhetzel at gmail.com> (from Sun, 14 Jan 2007  
14:54:28 -0600):

> On 1/14/07, Divacky Roman <xdivac02 at stud.fit.vutbr.cz> wrote:

>> +                       buf = (char *)(uintptr_t)args->arg2;
> :
>> +                       bsd_to_linux_fs(vfsp->vfc_name, &fs);
>> +                       len = strlen(fs.name) + 1;
>> +                       error = 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  
of the target buffer in some header? That would be very bad design.  
The kernel could overwrite userland data even if the kernel is not  
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  
buf should be.
---snip---

That's bad. That's very very bad. I don't like this in the FreeBSD  
kernel, I want an upper bound. Would you please try to find some  
artificial upper bound? The MFSNAMELEN would be one candidate. A  
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   
>> currently present
>> +                * in the kernel.
>> +                */
>> +               case 3:
>> +                       TAILQ_FOREACH(vfsp, &vfsconf, vfc_list)
>> +                               index++;
>> +                       td->td_retval[0] = 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  
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  
>>  fits? how do you
>> asure that?
>>
>
> name can't be larger than MFSNAMELEN, and sizeof(fs->name) == MFSNAMELEN.

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  
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  
linux_misc.c:linux_prctl(), specially the LINUX_PR_{SET|GET}_NAME case  
and the comments in there. What you are doing here should be done  
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  
you want to get write access to our perforce repository (that's not an  
official CVS or developer account, but a step in this direction)? If  
yes, what username do you prefer?

Bye,
Alexander.

-- 
I'm still waiting for the advent of the computer science groupie.

http://www.Leidinger.net    Alexander @ Leidinger.net: PGP ID = B0063FE7
http://www.FreeBSD.org       netchild @ FreeBSD.org  : PGP ID = 72077137