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

[Mateusz Guzik]

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.

-- 
Mateusz Guzik <mjguzik gmail.com>