linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: linux-cxl@vger.kernel.org, linux-acpi@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-nvdimm@lists.01.org,
	linux-pci@vger.kernel.org, Bjorn Helgaas <helgaas@kernel.org>,
	Chris Browy <cbrowy@avery-design.com>,
	Christoph Hellwig <hch@infradead.org>,
	Dan Williams <dan.j.williams@intel.com>,
	David Hildenbrand <david@redhat.com>,
	David Rientjes <rientjes@google.com>,
	Ira Weiny <ira.weiny@intel.com>, Jon Masters <jcm@jonmasters.org>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Rafael Wysocki <rafael.j.wysocki@intel.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	Vishal Verma <vishal.l.verma@intel.com>,
	"John Groves (jgroves)" <jgroves@micron.com>,
	"Kelley, Sean V" <sean.v.kelley@intel.com>,
	kernel test robot <lkp@intel.com>,
	Dan Williams <dan.j.willams@intel.com>
Subject: Re: [PATCH v2 4/8] cxl/mem: Add basic IOCTL interface
Date: Sun, 14 Feb 2021 23:50:12 +0000	[thread overview]
Message-ID: <YCm3NFqO9gVtXOZP@zeniv-ca.linux.org.uk> (raw)
In-Reply-To: <20210214231456.xnwitliczv6qwmjv@intel.com>

On Sun, Feb 14, 2021 at 03:14:56PM -0800, Ben Widawsky wrote:
> On 21-02-14 16:30:09, Al Viro wrote:
> > On Tue, Feb 09, 2021 at 04:02:55PM -0800, Ben Widawsky wrote:
> > 
> > > +static int handle_mailbox_cmd_from_user(struct cxl_memdev *cxlmd,
> > > +					const struct cxl_mem_command *cmd,
> > > +					u64 in_payload, u64 out_payload,
> > > +					struct cxl_send_command __user *s)
> > > +{
> > > +	struct cxl_mem *cxlm = cxlmd->cxlm;
> > > +	struct device *dev = &cxlmd->dev;
> > > +	struct mbox_cmd mbox_cmd = {
> > > +		.opcode = cmd->opcode,
> > > +		.size_in = cmd->info.size_in,
> > > +	};
> > > +	s32 user_size_out;
> > > +	int rc;
> > > +
> > > +	if (get_user(user_size_out, &s->out.size))
> > > +		return -EFAULT;
> > 
> > You have already copied it in.  Never reread stuff from userland - it *can*
> > change under you.
> 
> As it turns out, this is some leftover logic which doesn't need to exist at all,
> and I'm happy to change it. Thanks for reviewing.
> 
> I wasn't familiar with this restriction though. For my edification could you
> explain how that could happen? Also, is this something that should go in the
> kdocs, because I don't see anything about this restriction there.

Er...  You do realize that if two processes share memory, one can bloody well
modify it while another is in the middle of syscall, right?  Always could -
even mmap(2) with MAP_SHARED is sufficient, same as shmat(2), or the wholesale
sharing between POSIX threads, etc.

And even on UP with no preemption you could bloody well have a structure that
spans a page boundary, with the next page being mmapped and currently not
present in memory.  Then copy_from_user() would've copied the beginning, hit
a page fault, try to read the next page from something slow and lose CPU.
Letting the second process run and modify the already copied part.

It has been possible since at least mid-80s, well before Linux.  Anything in
user memory can change under you, right in the middle of syscall.  Always
could.  And there had been very real bugs along the lines of data being
read twice, once for safety check, once for actual work.  Something like

	get_user(len, &user_object->len);
	check that len is reasonable
	p = kmalloc(offsetof(struct foo, string[len]), GFP_KERNEL);
	copy_from_user(p, user_object, len);
	work with the copy, assuming that first p->len bytes of p->string[]
are safe to use, find out that p->len is much greater than len since
the userland data has changed between two fetches

Some of those had been exploitable from the very beginning, some had become
such on innocious-looking changes.

For the sake of your sanity it's better to avoid such landmines.  In some
cases it's OK to read the data twice (e.g. in something like select(2)), but
those cases are rare and seeing something of that sort is generally a big
red flag on review.  In almost all cases it's best avoided.

  reply	other threads:[~2021-02-14 23:51 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-10  0:02 [PATCH v2 0/8] CXL 2.0 Support Ben Widawsky
2021-02-10  0:02 ` [PATCH v2 1/8] cxl/mem: Introduce a driver for CXL-2.0-Type-3 endpoints Ben Widawsky
2021-02-10 16:17   ` Jonathan Cameron
2021-02-10 17:12     ` Ben Widawsky
2021-02-10 17:23       ` Jonathan Cameron
2021-02-10  0:02 ` [PATCH v2 2/8] cxl/mem: Find device capabilities Ben Widawsky
2021-02-10 13:32   ` Jonathan Cameron
2021-02-10 15:07     ` Jonathan Cameron
2021-02-10 16:55       ` Ben Widawsky
2021-02-10 17:30         ` Jonathan Cameron
2021-02-10 18:16         ` Ben Widawsky
2021-02-11  9:55           ` Jonathan Cameron
2021-02-11 15:55             ` Ben Widawsky
2021-02-12 13:27               ` Jonathan Cameron
2021-02-12 15:54                 ` Ben Widawsky
2021-02-11 18:27             ` Ben Widawsky
2021-02-12 13:23               ` Jonathan Cameron
2021-02-10 19:32     ` Ben Widawsky
2021-02-10 17:41   ` Jonathan Cameron
2021-02-10 18:53     ` Ben Widawsky
2021-02-10 19:54       ` Dan Williams
2021-02-11 10:01         ` Jonathan Cameron
2021-02-11 16:04           ` Ben Widawsky
2021-02-10  0:02 ` [PATCH v2 3/8] cxl/mem: Register CXL memX devices Ben Widawsky
2021-02-10 18:17   ` Jonathan Cameron
2021-02-11 10:17     ` Jonathan Cameron
2021-02-11 20:40       ` Dan Williams
2021-02-12 13:33         ` Jonathan Cameron
2021-02-10  0:02 ` [PATCH v2 4/8] cxl/mem: Add basic IOCTL interface Ben Widawsky
2021-02-10 18:45   ` Jonathan Cameron
2021-02-10 20:22     ` Ben Widawsky
2021-02-11  4:40     ` Dan Williams
2021-02-11 10:06       ` Jonathan Cameron
2021-02-11 16:54         ` Ben Widawsky
2021-02-14 16:30   ` Al Viro
2021-02-14 23:14     ` Ben Widawsky
2021-02-14 23:50       ` Al Viro [this message]
2021-02-14 23:57         ` Al Viro
2021-02-10  0:02 ` [PATCH v2 5/8] cxl/mem: Add a "RAW" send command Ben Widawsky
2021-02-10 15:26   ` Ariel.Sibley
2021-02-10 16:49     ` Ben Widawsky
2021-02-10 18:03       ` Ariel.Sibley
2021-02-10 18:11         ` Ben Widawsky
2021-02-10 18:46           ` Ariel.Sibley
2021-02-10 19:12             ` Ben Widawsky
2021-02-11 16:43     ` Dan Williams
2021-02-11 11:19   ` Jonathan Cameron
2021-02-11 16:01     ` Ben Widawsky
2021-02-12 13:40       ` Jonathan Cameron
2021-02-10  0:02 ` [PATCH v2 6/8] cxl/mem: Enable commands via CEL Ben Widawsky
2021-02-11 12:02   ` Jonathan Cameron
2021-02-11 17:45     ` Ben Widawsky
2021-02-11 20:34       ` Dan Williams
2021-02-16 13:43     ` Bartosz Golaszewski
2021-02-10  0:02 ` [PATCH v2 7/8] cxl/mem: Add set of informational commands Ben Widawsky
2021-02-11 12:07   ` Jonathan Cameron
2021-02-10  0:02 ` [PATCH v2 8/8] MAINTAINERS: Add maintainers of the CXL driver Ben Widawsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YCm3NFqO9gVtXOZP@zeniv-ca.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=ben.widawsky@intel.com \
    --cc=cbrowy@avery-design.com \
    --cc=dan.j.willams@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=david@redhat.com \
    --cc=hch@infradead.org \
    --cc=helgaas@kernel.org \
    --cc=ira.weiny@intel.com \
    --cc=jcm@jonmasters.org \
    --cc=jgroves@micron.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvdimm@lists.01.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lkp@intel.com \
    --cc=rafael.j.wysocki@intel.com \
    --cc=rdunlap@infradead.org \
    --cc=rientjes@google.com \
    --cc=sean.v.kelley@intel.com \
    --cc=vishal.l.verma@intel.com \
    --subject='Re: [PATCH v2 4/8] cxl/mem: Add basic IOCTL interface' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).