Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1420372 > unrolled thread

[PATCH 3.2 31/46] IB/security: Restrict use of the write() interface

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3.2 31/46] IB/security: Restrict use of the write() interface Ben Hutchings <ben@decadent.org.uk> - 2016-06-12 23:50 +0200
    Re: [PATCH 3.2 31/46] IB/security: Restrict use of the write() interface Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-06-14 23:20 +0200
      Re: [PATCH 3.2 31/46] IB/security: Restrict use of the write()  interface Ben Hutchings <ben@decadent.org.uk> - 2016-06-14 23:30 +0200
        Re: [PATCH 3.2 31/46] IB/security: Restrict use of the write() interface Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-06-15 00:10 +0200

#1420372 — [PATCH 3.2 31/46] IB/security: Restrict use of the write() interface

3.2.81-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

commit e6bd18f57aad1a2d1ef40e646d03ed0f2515c9e3 upstream.

The drivers/infiniband stack uses write() as a replacement for
bi-directional ioctl().  This is not safe. There are ways to
trigger write calls that result in the return structure that
is normally written to user space being shunted off to user
specified kernel memory instead.

For the immediate repair, detect and deny suspicious accesses to
the write API.

For long term, update the user space libraries and the kernel API
to something that doesn't present the same security vulnerabilities
(likely a structured ioctl() interface).

The impacted uAPI interfaces are generally only available if
hardware from drivers/infiniband is installed in the system.

Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
[ Expanded check to all known write() entry points ]
Signed-off-by: Doug Ledford <dledford@redhat.com>
[bwh: Backported to 3.2:
 - Drop changes to hfi1
 - include/rdma/ib.h didn't exist, so create it with the usual header guard
   and include it in drivers/infiniband/core/ucma.c
 - ipath_write() has the same problem, so add the same restriction there]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -48,6 +48,7 @@
 
 #include <asm/uaccess.h>
 
+#include <rdma/ib.h>
 #include <rdma/ib_cm.h>
 #include <rdma/ib_user_cm.h>
 #include <rdma/ib_marshall.h>
@@ -1116,6 +1117,9 @@ static ssize_t ib_ucm_write(struct file
 	struct ib_ucm_cmd_hdr hdr;
 	ssize_t result;
 
+	if (WARN_ON_ONCE(!ib_safe_file_access(filp)))
+		return -EACCES;
+
 	if (len < sizeof(hdr))
 		return -EINVAL;
 
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -47,6 +47,7 @@
 #include <rdma/ib_marshall.h>
 #include <rdma/rdma_cm.h>
 #include <rdma/rdma_cm_ib.h>
+#include <rdma/ib.h>
 
 MODULE_AUTHOR("Sean Hefty");
 MODULE_DESCRIPTION("RDMA Userspace Connection Manager Access");
@@ -1268,6 +1269,9 @@ static ssize_t ucma_write(struct file *f
 	struct rdma_ucm_cmd_hdr hdr;
 	ssize_t ret;
 
+	if (WARN_ON_ONCE(!ib_safe_file_access(filp)))
+		return -EACCES;
+
 	if (len < sizeof(hdr))
 		return -EINVAL;
 
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -48,6 +48,8 @@
 
 #include <asm/uaccess.h>
 
+#include <rdma/ib.h>
+
 #include "uverbs.h"
 
 MODULE_AUTHOR("Roland Dreier");
@@ -580,6 +582,9 @@ static ssize_t ib_uverbs_write(struct fi
 	struct ib_uverbs_file *file = filp->private_data;
 	struct ib_uverbs_cmd_hdr hdr;
 
+	if (WARN_ON_ONCE(!ib_safe_file_access(filp)))
+		return -EACCES;
+
 	if (count < sizeof hdr)
 		return -EINVAL;
 
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -45,6 +45,8 @@
 #include <linux/delay.h>
 #include <linux/export.h>
 
+#include <rdma/ib.h>
+
 #include "qib.h"
 #include "qib_common.h"
 #include "qib_user_sdma.h"
@@ -1971,6 +1973,9 @@ static ssize_t qib_write(struct file *fp
 	ssize_t ret = 0;
 	void *dest;
 
+	if (WARN_ON_ONCE(!ib_safe_file_access(fp)))
+		return -EACCES;
+
 	if (count < sizeof(cmd.type)) {
 		ret = -EINVAL;
 		goto bail;
--- /dev/null
+++ b/include/rdma/ib.h
@@ -0,0 +1,21 @@
+#if !defined(_RDMA_IB_H)
+#define _RDMA_IB_H
+
+#include <linux/sched.h>
+
+/*
+ * The IB interfaces that use write() as bi-directional ioctl() are
+ * fundamentally unsafe, since there are lots of ways to trigger "write()"
+ * calls from various contexts with elevated privileges. That includes the
+ * traditional suid executable error message writes, but also various kernel
+ * interfaces that can write to file descriptors.
+ *
+ * This function provides protection for the legacy API by restricting the
+ * calling context.
+ */
+static inline bool ib_safe_file_access(struct file *filp)
+{
+	return filp->f_cred == current_cred() && segment_eq(get_fs(), USER_DS);
+}
+
+#endif /* _RDMA_IB_H */
--- a/drivers/infiniband/hw/ipath/ipath_file_ops.c
+++ b/drivers/infiniband/hw/ipath/ipath_file_ops.c
@@ -44,6 +44,8 @@
 #include <linux/cpu.h>
 #include <asm/pgtable.h>
 
+#include <rdma/ib.h>
+
 #include "ipath_kernel.h"
 #include "ipath_common.h"
 #include "ipath_user_sdma.h"
@@ -2239,6 +2241,9 @@ static ssize_t ipath_write(struct file *
 	ssize_t ret = 0;
 	void *dest;
 
+	if (WARN_ON_ONCE(!ib_safe_file_access(fp)))
+		return -EACCES;
+
 	if (count < sizeof(cmd.type)) {
 		ret = -EINVAL;
 		goto bail;

[toc] | [next] | [standalone]

#1422345

On Sunday 12 June 2016 10:34 PM, Ben Hutchings wrote:
> 3.2.81-rc1 review patch.  If anyone has any objections, please let me know.
>
> ------------------
>
> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>
> commit e6bd18f57aad1a2d1ef40e646d03ed0f2515c9e3 upstream.
>
> The drivers/infiniband stack uses write() as a replacement for
> bi-directional ioctl().  This is not safe. There are ways to
> trigger write calls that result in the return structure that
> is normally written to user space being shunted off to user
> specified kernel memory instead.
>

<snip>

> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> --- a/drivers/infiniband/core/ucm.c
> +++ b/drivers/infiniband/core/ucm.c
> @@ -48,6 +48,7 @@
>
>   #include <asm/uaccess.h>
>
> +#include <rdma/ib.h>

This is breaking the build. There is no rdma/ib.h . The file was created by:
8d36eb01da5d ("RDMA/cma: Define native IB address")

build log is at: https://gitlab.com/sudipm/linux-next/builds/1771265

Regards
Sudip

[toc] | [prev] | [next] | [standalone]

#1422356 — Re: [PATCH 3.2 31/46] IB/security: Restrict use of the write() interface

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2016-06-14 at 22:11 +0100, Sudip Mukherjee wrote:
> On Sunday 12 June 2016 10:34 PM, Ben Hutchings wrote:
> > 3.2.81-rc1 review patch.  If anyone has any objections, please let
> > me know.
> > 
> > ------------------
> > 
> > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > 
> > commit e6bd18f57aad1a2d1ef40e646d03ed0f2515c9e3 upstream.
> > 
> > The drivers/infiniband stack uses write() as a replacement for
> > bi-directional ioctl().  This is not safe. There are ways to
> > trigger write calls that result in the return structure that
> > is normally written to user space being shunted off to user
> > specified kernel memory instead.
> > 
> 
> <snip>
> 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > ---
> > --- a/drivers/infiniband/core/ucm.c
> > +++ b/drivers/infiniband/core/ucm.c
> > @@ -48,6 +48,7 @@
> > 
> >   #include <asm/uaccess.h>
> > 
> > +#include <rdma/ib.h>
> 
> This is breaking the build. There is no rdma/ib.h .

This backported patch adds it.

>  The file was created by:
> 8d36eb01da5d ("RDMA/cma: Define native IB address")
> 
> build log is at: https://gitlab.com/sudipm/linux-next/builds/1771265

It looks like your patch queue tester doesn't account for patches that
create new files.

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert
Camus

[toc] | [prev] | [next] | [standalone]

#1422379

On Tuesday 14 June 2016 10:23 PM, Ben Hutchings wrote:
> On Tue, 2016-06-14 at 22:11 +0100, Sudip Mukherjee wrote:
>> On Sunday 12 June 2016 10:34 PM, Ben Hutchings wrote:
>>> 3.2.81-rc1 review patch.  If anyone has any objections, please let
>>> me know.
>>>
>>> ------------------
>>>
>>> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>>
>>> commit e6bd18f57aad1a2d1ef40e646d03ed0f2515c9e3 upstream.
>>>
>>> The drivers/infiniband stack uses write() as a replacement for
>>> bi-directional ioctl().  This is not safe. There are ways to
>>> trigger write calls that result in the return structure that
>>> is normally written to user space being shunted off to user
>>> specified kernel memory instead.
>>>
>>
>> <snip>
>>
>>> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
>>> ---
>>> --- a/drivers/infiniband/core/ucm.c
>>> +++ b/drivers/infiniband/core/ucm.c
>>> @@ -48,6 +48,7 @@
>>>
>>>    #include <asm/uaccess.h>
>>>
>>> +#include <rdma/ib.h>
>>
>> This is breaking the build. There is no rdma/ib.h .
>
> This backported patch adds it.
>
>>   The file was created by:
>> 8d36eb01da5d ("RDMA/cma: Define native IB address")
>>
>> build log is at: https://gitlab.com/sudipm/linux-next/builds/1771265
>
> It looks like your patch queue tester doesn't account for patches that
> create new files.

oops... after applying your combined diff I added them to git with
git add -u , and that doesnot take care of new files. sorry for the 
noise. I should have been more careful.

But the other failure is not noise. :)

Regards
Sudip

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web