[openbeosstorage] Re: [Open-beos-cvs]CVS: current/src/kernel/core/disk_device_managerddm_userland_interface.cpp,1.17,1.18

On 2003-09-27 at 07:19:39 [+0200], Tyler Dauwalder wrote:
> Update of /cvsroot/open-beos/current/src/kernel/core/disk_device_manager
> In directory 
> sc8-pr-cvs1:/tmp/cvs-serv26800/src/kernel/core/disk_device_manager
> 
> Modified Files:
>     ddm_userland_interface.cpp
> Log Message:
> Made first half of so of ddm syscalls kernel-safe.
> 
> 
> Index: ddm_userland_interface.cpp
> CVS Browser:
> http://cvs.sourceforge.net/cgi-bin/viewcvs.cgi/open-beos/current/src/kernel/core/disk_device_manager/ddm_userland_interface.cpp
> ===================================================================
> RCS file: 
> /cvsroot/open-beos/current/src/kernel/core/disk_device_manager/ddm_userland_interface.cpp,v
> retrieving revision 1.17
> retrieving revision 1.18
> diff -u -d -r1.17 -r1.18
> --- ddm_userland_interface.cpp    21 Sep 2003 21:26:12 -0000    1.17
> +++ ddm_userland_interface.cpp    27 Sep 2003 05:19:37 -0000    1.18
> @@ -9,6 +9,8 @@
>  #include <KDiskSystem.h>
>  #include <KFileDiskDevice.h>
>  #include <KShadowPartition.h>
> +#include <malloc.h>
> +#include <vm.h>
>  
>  #include "KDiskDeviceJobGenerator.h"
>  #include "UserDataWriter.h"
> @@ -457,14 +459,16 @@
>  
>  // _kern_get_next_disk_device_id
>  partition_id
> -_kern_get_next_disk_device_id(int32 *cookie, size_t *neededSize)
> +_kern_get_next_disk_device_id(int32 *_cookie, size_t *neededSize)
>  {
> -    if (!cookie)
> +    if (!_cookie)
>          return B_BAD_VALUE;
> +    int32 cookie = *_cookie;
> +    
>      partition_id id = B_ENTRY_NOT_FOUND;
>      KDiskDeviceManager *manager = KDiskDeviceManager::Default();
>      // get the next device
> -    if (KDiskDevice *device = manager->RegisterNextDevice(cookie)) {
> +    if (KDiskDevice *device = manager->RegisterNextDevice(&cookie)) {
>          PartitionRegistrar _(device, true);
>          id = device->ID();
>          if (neededSize) {
> @@ -473,19 +477,26 @@
>                  UserDataWriter writer;
>                  device->WriteUserData(writer, false);
>                  *neededSize = writer.AllocatedSize();
> -            } else
> -                return B_ERROR;
> +            } else {
> +                id = B_ERROR;
> +            }
>          }
>      }
> +    *_cookie = cookie;
>      return id;
>  }

I'm afraid, the cookie handling is not sufficient. At any rate userland memory 
access should only happen through user_memcpy(). 
E.g. in user_get_next_port_info():

        if ((addr)ucookie >= KERNEL_BASE && (addr)ucookie <= KERNEL_TOP)
                return ERR_VM_BAD_USER_MEMORY;
        [...]
        // copy from userspace
        rc = user_memcpy(&cookie, ucookie, sizeof(int32));
        if (rc < 0)
                return rc;
        [...]
        res = get_next_port_info(uteam, &cookie, &info);
        [...]   
        rc = user_memcpy(ucookie, &cookie, sizeof(int32));
        if (rc < 0)
                return rc;

This can probably be simplified a bit by introducing template functions for 
dealing with reference variables copied from 
userland. They may look like this:

template<typename T>
inline
status_t
copy_ref_var_from_user(T *user, T &kernel)
{
        if ((addr)user >= KERNEL_BASE && (addr)user <= KERNEL_TOP)
                return ERR_VM_BAD_USER_MEMORY;
        return user_memcpy(&kernel, user, sizeof(T));
}

template<typename T>
inline
status_t
copy_ref_var_to_user(T &kernel, T *user)
{
        return user_memcpy(user, &kernel, sizeof(T));
}

The latter does little, but provides a bit more type safety, which makes bugs 
like the one I just fixed in 
user_get_next_port_info() (info was copied into ucookie) less likely.

[...]
> @@ -538,11 +554,20 @@
>  // _kern_get_disk_device_data
>  status_t
>  _kern_get_disk_device_data(partition_id id, bool deviceOnly, bool shadow,
> -                           user_disk_device_data *buffer, size_t 
> bufferSize,
> +                           user_disk_device_data *_buffer, size_t 
> bufferSize,
>                             size_t *neededSize)
>  {
> -    if (!buffer && bufferSize > 0)
> +    if (!_buffer && bufferSize > 0)
>          return B_BAD_VALUE;
> +
> +    // copy in
> +    user_disk_device_data *buffer = bufferSize > 0
> +                                    ? 
> reinterpret_cast<user_disk_device_data*>(malloc(bufferSize))

static_cast should be sufficient, BTW. No need for the sledgehammer. ;-)

However, we should be more careful here. Since bufferSize may have a huge 
value, we shouldn't malloc() kernel memory unchecked. 
So, either we set a reasonably great size constraint (100KB or so maybe) or do 
a dry run with the UserDataWriter to get the 
exactly required size. I'd prefer the latter solution.

> +                                    : NULL;
> +    if (buffer)
> +        user_memcpy(buffer, _buffer, bufferSize);
> +        
> +    status_t result = B_OK;    
>      KDiskDeviceManager *manager = KDiskDeviceManager::Default();
>      // get the device
>      if (KDiskDevice *device = manager->RegisterDevice(id, deviceOnly)) {
> @@ -553,47 +578,76 @@
>              device->WriteUserData(writer, shadow);
>              if (neededSize)
>                  *neededSize = writer.AllocatedSize();
> -            if (writer.AllocatedSize() <= bufferSize)
> -                return B_OK;
> -            return B_BUFFER_OVERFLOW;
> +            if (writer.AllocatedSize() > bufferSize)
> +                result = B_BUFFER_OVERFLOW;
>          }
> +    } else {
> +        result = B_ENTRY_NOT_FOUND;
>      }
> -    return B_ENTRY_NOT_FOUND;
> +
> +    // copy out
> +    if (result == B_OK && buffer)
> +        user_memcpy(_buffer, buffer, bufferSize);
> +    free(buffer);
> +    
> +    return result;
>  }

Damn, I didn't think of that -- I'm afraid this doesn't work (haven't tested it 
yet, since I get strange compilation error, like 
`.../KFileDiskDevice.cpp:188: implicit declaration of function `int 
remove(...)'' although <stdio.h> is included -- will track 
this down later).
The problem is, that user_disk_device_data/user_partition_data make use of 
pointers into the same buffer, that would need to be 
relocated when copied back into the userland buffer.

There are at least two solutions:

1) IIRC, Axel once mentioned, that one could lock userland memory and wouldn't 
need to do the copying into kernel memory and 
back, then. I don't remember what the status of this feature was. Unimplemented?

2) Relocate the pointers. It shouldn't be too hard to add the required 
relocation functionality to UserDataWriter and make the 
concerned classes use it.

Given that the amount of memory to be copied is not that big (I guess usually 
less than 500 bytes per partition) solution 2) 
should be fine. Since I need the test working, I'll fix that as soon as I 
managed to track the compilation error down.

CU, Ingo

Other related posts: