From owner-svn-src-head@freebsd.org Wed Dec 2 20:54:22 2015 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D28A9A3FB83; Wed, 2 Dec 2015 20:54:22 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm0-x22b.google.com (mail-wm0-x22b.google.com [IPv6:2a00:1450:400c:c09::22b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6A7D41439; Wed, 2 Dec 2015 20:54:22 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by wmec201 with SMTP id c201so75481684wme.1; Wed, 02 Dec 2015 12:54:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=wlHgv7hyBdVHXnL5pV75x8EcZcvCvWCtTyooNo3lfuU=; b=mQ2NfZ88apLrvD3LrCUOLMgqcsFDbZqlmQZp9M4n+sAV43E6/GLGOBSIbTHQetgPci /JLus8vz4WYOm7eu+NgnSJZLRaApPDEXL9aeki3H9gE0noC5K4k4SLIZ6ST2QyvMqWEX aYod46sK2TrI8Cr9azby67agv6myxwkFnSFckB4iKtkVKr+1y30iQ6lCFvbMWhcpXpDV 9CNsuc23c+hHgedsFJrGMAHSngBAgiwbFtTMPC8rtTPuh73ibYRDWjlPinxb2heRD8qq rcvzsTeMAQWQ4wAQPXOaz4AoWvMwNeGI6RIvlvsAs4WzpbOnW83uGZR2q+T+YFWmAckE MsrQ== X-Received: by 10.28.105.23 with SMTP id e23mr48476537wmc.80.1449089660985; Wed, 02 Dec 2015 12:54:20 -0800 (PST) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by smtp.gmail.com with ESMTPSA id a63sm32514961wmc.5.2015.12.02.12.54.19 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 02 Dec 2015 12:54:19 -0800 (PST) Date: Wed, 2 Dec 2015 21:54:18 +0100 From: Mateusz Guzik To: Hans Petter Selasky Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r291481 - head/sys/compat/linuxkpi/common/include/linux Message-ID: <20151202205417.GC30250@dft-labs.eu> References: <201511300924.tAU9OC7o049788@repo.freebsd.org> <20151202202958.GA30250@dft-labs.eu> <565F58EF.1030707@selasky.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <565F58EF.1030707@selasky.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Dec 2015 20:54:22 -0000 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