Re: user_memcpy()

> Hi there,
> 
> François Revol made me recognize a design flaw in our user_memcpy()/
> user_strlcpy() API: you have to check the validity of the user 
> address 
> yourself, ie. you have to make sure it's in user space.
> But we don't have a function for this, just the (kernel private) 
> macro 
> IS_USER_ADDRESS().

Actually there is more to that...
you will have to check addresses differently if the call is being made 
by the app or the kernel itself, that is if a driver called open().
I an app calls ioctl(), it goes through the syscall, gets to 
user_ioctl(), then to your driver, where you check the addresses 
against IS_USER_ADDRESS(). But what when the kernel itself calls its 
own ioctl() ? If you go the same way you will simply reject the call as 
IS_USER_ADDRESS will return false because the kernel supplied a buffer 
from kernel space.

While R5 provided something to validate addresses (and make sure they 
don't go away), it didn't have anything to tell you what to actually 
check.
So you end up having is_valid_range(), which noone called anyway (also 
because it wasn't in a public header), and to which you didn't know 
which flag to give anyway:

// from my sockfs header:

/*
#define B_READ_AREA 1
#define B_WRITE_AREA 2 */
#define B_USER_READ_AREA    0x00000004  /* user can read */
#define B_USER_WRITE_AREA    0x00000008  /* user can write */

/* why isn't that in KernelExport ??? */
extern bool is_valid_range(void *start, size_t len, uint32 prot);

I defined that macro to calc the prot bits needed:

#define PROT(kern,rw) ( rw ? (kern ? (B_WRITE_AREA | B_READ_AREA) : \
    (B_USER_WRITE_AREA | B_USER_READ_AREA)) : \
    (kern ? B_READ_AREA : B_USER_READ_AREA) )

but as I said there is no way to tell the kern arg, so currently it's 
hardcoded to 1, so kernel calls can work, but of course user calls are 
not checked correctly and can leed to trashing kernel data.

Another problem of not being able to tell wether a call is made from 
user or kernel side, is that you don't know what to use when checking 
for uids and other perms. Of course it's not really of concerns for 
now, but well mayeb it's time for it?
Like, what should an fs addon call like creat() do when it's called by 
the kernel and it sees the current uid is not permitted to write in wd 
?
The kernel doesn't care about that, it's not what it asked the fs to 
do. That is the kernel is always root, and should act as such.

The solution used in Linux (at least to teh address problem) is to have 
a pointer in the thread structure, that tells the maximum address that 
is currently allowed to be used when passing pointers to calls. 
(remember Linux has the kernel at the end of the vm).
When the kernel does in its own call is save current->addr_limit to the 
stack, and set it to KERNEL_DS, that is the last byte of the virtual 
space. Then it puts back what it saved, which was USER_DS if the 
previous call was from a user fd.
So for example, a write() from a socket would do:
user_write(s) // addr_limit is USER_DS
//some protocol stuff...
write(nic_fd); // addr_limit is still USER_FD
{
    addr_t saved = current.addr_limit;
    addr_limit = KERNEL_DS;
    sys_write(nic_fd);
    addr_limit = saved;
}

See:
http://lxr.linux.no/source/include/asm-i386/uaccess.h

Then there is the whole list of *_to/from_user funcs which use nasty 
tricks with the exception handler and the elf file sections (.eh_frame 
or something).
When an exception occurs, the handler first checks if eip points into 
one of the functions listed in that .eh_foo section, if yes it simply 
returns back to a specific address in that function which returns an 
error. Think setjmp for segfaults.

> And while I like to keep the macro, we should at least export an 
> is_user_address() function.

Now, the elf loader in BeOS certainly doesn't care about those 
sections, and the exception handler doesn't either. Probably Haiku has/
will get something like that, but it would be nice to have something 
that could be used today in drivers for R5, which at most would only 
need a recompile to get a correct behaviour from it in Haiku and Zeta.

Another thing is the driver shouldn't have to know about the address 
space map, that is it shouldn't hardocde any address, certainly not the 
PAGE_OFFSET like in Linux. Linux drivers use this (through the macros), 
because they have teh source of the kernel around, but we a) don't have 
the source of some of the platforms in questions, b) don't want the 
drivers to be that much tied to the kernel, c) want the driver to still 
work if we boot a new kernel that has say a new 1:3 vm split, or maybe 
a reverse split as it would make it easier for WINE.

Now, I guess the copy_to/from_user funcs can probably do the checks 
themselves.
An R5 implementation of copy_to_user could be:

status_t copy_to_user(void *to, void *from, size_t len)
{
	if (!is_valid_range(to, len, B_WRITE_AREA))
        return EFAULT;
    return memcpy(to, from, len);
}

While Haiku's could be something closer to the linux one:
status_t copy_to_user(void *to, void *from, size_t len)
{
	if (to < current->addr_limit)
        return EFAULT;
    // set up exception handler for memcpy
    return memcpy(to, from, len);
}
(but there is still the question of copying to an address in user space 
that is valid, but read only...)

And in Zeta I could for now at least do something like
adding a field named "user_call" for ex and:
status_t copy_to_user(void *to, void *from, size_t len)
{
    thread_rec *thr = get_curr_thr();
	if (!is_valid_range(to, len, 
                thr->user_call?B_USER_WRITE_AREA:B_WRITE_AREA))
        return EFAULT;
    return memcpy(to, from, len);
}

Then there is the need to know who called us, to check for perms.
Since R5 doesn't have that, We have to hardcode it. But, if we hardcode 
it to "kernel called us" like for address checking we'll end up giving 
everyone root perms. Yeah I hear you singing R5 is not multiuser, but 
that's not technically correct :)
Anyway I'd propose 2 simple calls we could hardcode as macros for R5:

#if defined(COMPILE_FOR_R5) && !defined(B_ZETA_VERSION_VENTURE)
#define check_for_user() false
#define check_for_user_addr() false
#define check_for_user_fd() true
#else
// check for valid user addresses 
extern bool check_for_user_addr(void);
// check for valid user permissions (for fds and such)
extern bool check_for_user_fd(void);
#endif

The first one should hardly be needed, just used internaly by the 
*to/from_user() funcs, the later being useful to implement correct 
permisison checking without having to wait for the kernel to actually 
respect them.

I think I can stick something in Zeta R1, but the time window is 
shrinking, so it would be nice to reach a consensus there...

Comments ?

> 
> But that would still open a possible source of errors - as you still 
> have to check the address yourself. François suggested to do 
> something 
> similar than Linux that has copy_to_user() and copy_from_user() 
> functions.
> Since we also have a user_strlcpy() function, I would suggest the 
> following:
> 	copy_to_user()
> 	copy_from_user()
> 	strlcpy_to_user()
> 	strlcpy_from_user()
> 	user_memset() - or memset_user()?

Those I can add as well, same time considerations.

François.




-------------------------------------------------------
This SF.Net email is sponsored by: NEC IT Guy Games.
Get your fingers limbered up and give it your best shot. 4 great events, 4
opportunities to win big! Highest score wins.NEC IT Guy Games. Play to
win an NEC 61 plasma display. Visit http://www.necitguy.com/?r