svn commit: r291481 - head/sys/compat/linuxkpi/common/include/linux

[Hans Petter Selasky]

On 12/02/15 21:54, Mateusz Guzik wrote:
> On Wed, Dec 02, 2015 at 09:47:43PM +0100, Hans Petter Selasky wrote:
>> On 12/02/15 21:29, Mateusz Guzik wrote:
>>> On Mon, Nov 30, 2015 at 09:24:12AM +0000, Hans Petter Selasky wrote:
>>>> Author: hselasky
>>>> Date: Mon Nov 30 09:24:12 2015
>>>> New Revision: 291481
>>>> URL: https://svnweb.freebsd.org/changeset/base/291481
>>>>
>>>> Log:
>>>>    Add more functions and types to the LinuxKPI.
>>>>
>>>>    MFC after:	1 week
>>>>    Sponsored by:	Mellanox Technologies
>>>>
>>>> Modified:
>>>>    head/sys/compat/linuxkpi/common/include/linux/file.h
>>>>    head/sys/compat/linuxkpi/common/include/linux/workqueue.h
>>>>
>>>> Modified: head/sys/compat/linuxkpi/common/include/linux/file.h
>>>> ==============================================================================
>>>> --- head/sys/compat/linuxkpi/common/include/linux/file.h	Mon Nov 30 09:13:04 2015	(r291480)
>>>> +++ head/sys/compat/linuxkpi/common/include/linux/file.h	Mon Nov 30 09:24:12 2015	(r291481)
>>>> @@ -2,7 +2,7 @@
>>>>    * Copyright (c) 2010 Isilon Systems, Inc.
>>>>    * Copyright (c) 2010 iX Systems, Inc.
>>>>    * Copyright (c) 2010 Panasas, Inc.
>>>> - * Copyright (c) 2013 Mellanox Technologies, Ltd.
>>>> + * Copyright (c) 2013-2015 Mellanox Technologies, Ltd.
>>>>    * All rights reserved.
>>>>    *
>>>>    * Redistribution and use in source and binary forms, with or without
>>>> @@ -125,6 +125,21 @@ get_unused_fd(void)
>>>>   	return fd;
>>>>   }
>>>>
>>>> +static inline int
>>>> +get_unused_fd_flags(int flags)
>>>> +{
>>>> +	struct file *file;
>>>> +	int error;
>>>> +	int fd;
>>>> +
>>>> +	error = falloc(curthread, &file, &fd, flags);
>>>> +	if (error)
>>>> +		return -error;
>>>> +	/* drop the extra reference */
>>>> +	fdrop(file, curthread);
>>>> +	return fd;
>>>> +}
>>>> +
>>>
>>> This does not look right.
>>>
>>> AFAIR Linux drivers are not going to install fds into kernel threads. So
>>> this would be used for a userspace thread, but then it would completely
>>> insecure.
>>>
>>> Linux model is to reserve a slot in the fd table, obtain a 'file' object
>>> and install it as the last step.
>>>
>>> FreeBSD installs the file right away, but this means an extra reference
>>> has to be held in case something else using the table closes the fd.
>>>
>>> As such, this fdrop can lead to a use-after-free as the file can be
>>> freed from this poin.
>>>
>>> I'm afraid there is no way around patching improted consumers.
>>>
>>
>> Hi Mateusz,
>>
>> Thanks for your input. Yes, there is a potential race there, but no
>> use-after-free from what I can see, because the LinuxKPI always
>> retrieve the file pointer by the file number using "fget_unlocked()".
>>
>> I'll look into if we can delay the fdrop() until after the fd_install().
>>
>
> I grepped an example consumer:
>
>> ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file,
>>                                        struct ib_device *ib_dev,
>>                                        const char __user *buf, int in_len,
>>                                        int out_len)
>> {
>>          struct ib_uverbs_create_comp_channel       cmd;
>>          struct ib_uverbs_create_comp_channel_resp  resp;
>>          struct file                               *filp;
>>          int ret;
>>
>>          if (out_len < sizeof resp)
>>                  return -ENOSPC;
>>
>>          if (copy_from_user(&cmd, buf, sizeof cmd))
>>                  return -EFAULT;
>>
>>          ret = get_unused_fd_flags(O_CLOEXEC);
>>          if (ret < 0)
>>                  return ret;
>>          resp.fd = ret;
>>
>
> So this is supposed to get the slot.
>
>>          filp = ib_uverbs_alloc_event_file(file, ib_dev, 0);
>
> file object is allocated separately.
>
>>          if (IS_ERR(filp)) {
>>                  put_unused_fd(resp.fd);
>>                  return PTR_ERR(filp);
>>          }
>>
>>          if (copy_to_user((void __user *) (unsigned long) cmd.response,
>>                           &resp, sizeof resp)) {
>>                  put_unused_fd(resp.fd);
>>                  fput(filp);
>>                  return -EFAULT;
>>          }
>>
>>          fd_install(resp.fd, filp);
>
> And here is the install.
>
>>          return in_len;
>> }
>
> If you drop, and have some magic to grab the file obtained from
> get_unused_fd_flags, the file object is unsafe to use.
>
> In general, I don't see how you can get around having to edit such
> functions. If they get edited, you can just use the FreeBSD scheme.
>
> Fortunately as far as error handling goes, it behaves the same way.
>

Hi,

I've made a patch here:

https://reviews.freebsd.org/D4351

Does it make sense?

--HPS