LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
@ 2018-06-15 15:23 Jann Horn
  2018-06-15 16:40 ` Al Viro
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Jann Horn @ 2018-06-15 15:23 UTC (permalink / raw)
  To: Jens Axboe, FUJITA Tomonori, Doug Gilbert, James E.J. Bottomley,
	Martin K. Petersen, linux-block, linux-scsi, jannh
  Cc: linux-kernel, Al Viro, kernel-hardening, security

As Al Viro noted in commit 128394eff343 ("sg_write()/bsg_write() is not fit
to be called under KERNEL_DS"), sg and bsg improperly access userspace
memory outside the provided buffer, permitting kernel memory corruption via
splice().
But they don't just do it on ->write(), also on ->read() and (in the case
of bsg) even on ->release().

As a band-aid, make sure that the ->read() and ->write() handlers can not
be called in weird contexts (kernel context or credentials different from
file opener), like for ib_safe_file_access().
Also, completely prevent user memory accesses from ->release().

If someone needs to use these interfaces from different security contexts,
a new interface should be written that goes through the ->ioctl() handler.

I've mostly copypasted ib_safe_file_access() over as
scsi_safe_file_access() because I couldn't find a good common header -
please tell me if you know a better way.
The duplicate pr_err_once() calls are so that each of them fires once;
otherwise, this would probably have to be a macro.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: <stable@vger.kernel.org>
Signed-off-by: Jann Horn <jannh@google.com>
---

I'm CC-ing security@ on this patch in case someone cares a lot, but
since you already need to have some pretty high privileges to use these
devices in the first place, I think this can be handled publicly.

In case anyone is interested in how I found these: I was looking at a
reverse callgraph of __might_fault and spotted the ->release handler of
block/bsg.c in there.

 block/bsg-lib.c          |  5 ++++-
 block/bsg.c              | 29 +++++++++++++++++++++--------
 drivers/scsi/sg.c        | 11 ++++++++++-
 include/linux/bsg.h      |  3 ++-
 include/scsi/scsi_cmnd.h | 19 +++++++++++++++++++
 5 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/block/bsg-lib.c b/block/bsg-lib.c
index 9419def8c017..cf5d4fdddbeb 100644
--- a/block/bsg-lib.c
+++ b/block/bsg-lib.c
@@ -53,7 +53,8 @@ static int bsg_transport_fill_hdr(struct request *rq, struct sg_io_v4 *hdr,
 	return 0;
 }
 
-static int bsg_transport_complete_rq(struct request *rq, struct sg_io_v4 *hdr)
+static int bsg_transport_complete_rq(struct request *rq, struct sg_io_v4 *hdr,
+		bool cleaning_up)
 {
 	struct bsg_job *job = blk_mq_rq_to_pdu(rq);
 	int ret = 0;
@@ -79,6 +80,8 @@ static int bsg_transport_complete_rq(struct request *rq, struct sg_io_v4 *hdr)
 	if (job->reply_len && hdr->response) {
 		int len = min(hdr->max_response_len, job->reply_len);
 
+		if (unlikely(cleaning_up))
+			ret = -EINVAL;
 		if (copy_to_user(uptr64(hdr->response), job->reply, len))
 			ret = -EFAULT;
 		else
diff --git a/block/bsg.c b/block/bsg.c
index 132e657e2d91..e64ef807d2d0 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -159,7 +159,8 @@ static int bsg_scsi_fill_hdr(struct request *rq, struct sg_io_v4 *hdr,
 	return 0;
 }
 
-static int bsg_scsi_complete_rq(struct request *rq, struct sg_io_v4 *hdr)
+static int bsg_scsi_complete_rq(struct request *rq, struct sg_io_v4 *hdr,
+		bool cleaning_up)
 {
 	struct scsi_request *sreq = scsi_req(rq);
 	int ret = 0;
@@ -179,7 +180,9 @@ static int bsg_scsi_complete_rq(struct request *rq, struct sg_io_v4 *hdr)
 		int len = min_t(unsigned int, hdr->max_response_len,
 					sreq->sense_len);
 
-		if (copy_to_user(uptr64(hdr->response), sreq->sense, len))
+		if (cleaning_up)
+			ret = -EINVAL;
+		else if (copy_to_user(uptr64(hdr->response), sreq->sense, len))
 			ret = -EFAULT;
 		else
 			hdr->response_len = len;
@@ -383,11 +386,12 @@ static struct bsg_command *bsg_get_done_cmd(struct bsg_device *bd)
 }
 
 static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
-				    struct bio *bio, struct bio *bidi_bio)
+				    struct bio *bio, struct bio *bidi_bio,
+				    bool cleaning_up)
 {
 	int ret;
 
-	ret = rq->q->bsg_dev.ops->complete_rq(rq, hdr);
+	ret = rq->q->bsg_dev.ops->complete_rq(rq, hdr, cleaning_up);
 
 	if (rq->next_rq) {
 		blk_rq_unmap_user(bidi_bio);
@@ -453,7 +457,7 @@ static int bsg_complete_all_commands(struct bsg_device *bd)
 			break;
 
 		tret = blk_complete_sgv4_hdr_rq(bc->rq, &bc->hdr, bc->bio,
-						bc->bidi_bio);
+						bc->bidi_bio, true);
 		if (!ret)
 			ret = tret;
 
@@ -488,7 +492,7 @@ __bsg_read(char __user *buf, size_t count, struct bsg_device *bd,
 		 * bsg_complete_work() cannot do that for us
 		 */
 		ret = blk_complete_sgv4_hdr_rq(bc->rq, &bc->hdr, bc->bio,
-					       bc->bidi_bio);
+					       bc->bidi_bio, false);
 
 		if (copy_to_user(buf, &bc->hdr, sizeof(bc->hdr)))
 			ret = -EFAULT;
@@ -532,6 +536,12 @@ bsg_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	int ret;
 	ssize_t bytes_read;
 
+	if (!scsi_safe_file_access(file)) {
+		pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
+			__func__, task_tgid_vnr(current), current->comm);
+		return -EINVAL;
+	}
+
 	bsg_dbg(bd, "read %zd bytes\n", count);
 
 	bsg_set_block(bd, file);
@@ -608,8 +618,11 @@ bsg_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 
 	bsg_dbg(bd, "write %zd bytes\n", count);
 
-	if (unlikely(uaccess_kernel()))
+	if (!scsi_safe_file_access(file)) {
+		pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
+			__func__, task_tgid_vnr(current), current->comm);
 		return -EINVAL;
+	}
 
 	bsg_set_block(bd, file);
 
@@ -859,7 +872,7 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		at_head = (0 == (hdr.flags & BSG_FLAG_Q_AT_TAIL));
 		blk_execute_rq(bd->queue, NULL, rq, at_head);
-		ret = blk_complete_sgv4_hdr_rq(rq, &hdr, bio, bidi_bio);
+		ret = blk_complete_sgv4_hdr_rq(rq, &hdr, bio, bidi_bio, false);
 
 		if (copy_to_user(uarg, &hdr, sizeof(hdr)))
 			return -EFAULT;
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 53ae52dbff84..997e06a22527 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -393,6 +393,12 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 	struct sg_header *old_hdr = NULL;
 	int retval = 0;
 
+	if (!scsi_safe_file_access(filp)) {
+		pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
+			__func__, task_tgid_vnr(current), current->comm);
+		return -EINVAL;
+	}
+
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp,
@@ -581,8 +587,11 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 	sg_io_hdr_t *hp;
 	unsigned char cmnd[SG_MAX_CDB_SIZE];
 
-	if (unlikely(uaccess_kernel()))
+	if (!scsi_safe_file_access(filp)) {
+		pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
+			__func__, task_tgid_vnr(current), current->comm);
 		return -EINVAL;
+	}
 
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
 		return -ENXIO;
diff --git a/include/linux/bsg.h b/include/linux/bsg.h
index dac37b6e00ec..c22bc359552a 100644
--- a/include/linux/bsg.h
+++ b/include/linux/bsg.h
@@ -11,7 +11,8 @@ struct bsg_ops {
 	int	(*check_proto)(struct sg_io_v4 *hdr);
 	int	(*fill_hdr)(struct request *rq, struct sg_io_v4 *hdr,
 				fmode_t mode);
-	int	(*complete_rq)(struct request *rq, struct sg_io_v4 *hdr);
+	int	(*complete_rq)(struct request *rq, struct sg_io_v4 *hdr,
+				bool cleaning_up);
 	void	(*free_rq)(struct request *rq);
 };
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index aaf1e971c6a3..d22118a38aa4 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -8,6 +8,8 @@
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/scatterlist.h>
+#include <linux/cred.h> /* for scsi_safe_file_access() */
+#include <linux/fs.h> /* for scsi_safe_file_access() */
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_request.h>
 
@@ -363,4 +365,21 @@ static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
 	return xfer_len;
 }
 
+/*
+ * The SCSI interfaces that use read() and write() as an asynchronous variant of
+ * ioctl(..., SG_IO, ...) are fundamentally unsafe, since there are lots of ways
+ * to trigger read() and write() calls from various contexts with elevated
+ * privileges. This can lead to kernel memory corruption (e.g. if these
+ * interfaces are called through splice()) and privilege escalation inside
+ * userspace (e.g. if a process with access to such a device passes a file
+ * descriptor to a SUID binary as stdin/stdout/stderr).
+ *
+ * This function provides protection for the legacy API by restricting the
+ * calling context.
+ */
+static inline bool scsi_safe_file_access(struct file *filp)
+{
+	return filp->f_cred == current_cred() && !uaccess_kernel();
+}
+
 #endif /* _SCSI_SCSI_CMND_H */
-- 
2.18.0.rc1.244.gcf134e6275-goog


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 15:23 [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release Jann Horn
@ 2018-06-15 16:40 ` Al Viro
  2018-06-15 16:44   ` Jann Horn
  2018-06-15 20:47   ` Douglas Gilbert
  2018-06-15 16:49 ` Al Viro
  2018-06-21 12:40 ` Christoph Hellwig
  2 siblings, 2 replies; 23+ messages in thread
From: Al Viro @ 2018-06-15 16:40 UTC (permalink / raw)
  To: Jann Horn
  Cc: Jens Axboe, FUJITA Tomonori, Doug Gilbert, James E.J. Bottomley,
	Martin K. Petersen, linux-block, linux-scsi, linux-kernel,
	kernel-hardening, security

On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:

> I've mostly copypasted ib_safe_file_access() over as
> scsi_safe_file_access() because I couldn't find a good common header -
> please tell me if you know a better way.
> The duplicate pr_err_once() calls are so that each of them fires once;
> otherwise, this would probably have to be a macro.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jann Horn <jannh@google.com>
> ---

WTF do you mean, in ->release()?  That makes no sense whatsoever -
what kind of copy_{to,from}_user() would be possible in there?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 16:40 ` Al Viro
@ 2018-06-15 16:44   ` Jann Horn
  2018-06-15 16:53     ` Al Viro
  2018-06-15 20:47   ` Douglas Gilbert
  1 sibling, 1 reply; 23+ messages in thread
From: Jann Horn @ 2018-06-15 16:44 UTC (permalink / raw)
  To: Al Viro
  Cc: axboe, fujita.tomonori, dgilbert, jejb, martin.petersen,
	linux-block, linux-scsi, kernel list, Kernel Hardening, security

On Fri, Jun 15, 2018 at 6:40 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
>
> > I've mostly copypasted ib_safe_file_access() over as
> > scsi_safe_file_access() because I couldn't find a good common header -
> > please tell me if you know a better way.
> > The duplicate pr_err_once() calls are so that each of them fires once;
> > otherwise, this would probably have to be a macro.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Jann Horn <jannh@google.com>
> > ---
>
> WTF do you mean, in ->release()?  That makes no sense whatsoever -
> what kind of copy_{to,from}_user() would be possible in there?

bsg_release -> bsg_put_device -> bsg_complete_all_commands ->
blk_complete_sgv4_hdr_rq -> bsg_scsi_complete_rq -> copy_to_user.
I don't think that was intentional.

Basically, the sense buffer is copied to a userspace address supplied
in the previous ->write() when you ->read() the reply. But when you
->release() the file without reading the reply, they have to clean it
up, and for that, they reuse the same code they use for ->read() - so
the sense buffer is written to userspace on ->release().

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 15:23 [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release Jann Horn
  2018-06-15 16:40 ` Al Viro
@ 2018-06-15 16:49 ` Al Viro
  2018-06-15 16:58   ` Jann Horn
  2018-06-21 12:40 ` Christoph Hellwig
  2 siblings, 1 reply; 23+ messages in thread
From: Al Viro @ 2018-06-15 16:49 UTC (permalink / raw)
  To: Jann Horn
  Cc: Jens Axboe, FUJITA Tomonori, Doug Gilbert, James E.J. Bottomley,
	Martin K. Petersen, linux-block, linux-scsi, linux-kernel,
	kernel-hardening, security

On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
> As Al Viro noted in commit 128394eff343 ("sg_write()/bsg_write() is not fit
> to be called under KERNEL_DS"), sg and bsg improperly access userspace
> memory outside the provided buffer, permitting kernel memory corruption via
> splice().
> But they don't just do it on ->write(), also on ->read() and (in the case
> of bsg) even on ->release().
> 
> As a band-aid, make sure that the ->read() and ->write() handlers can not
> be called in weird contexts (kernel context or credentials different from
> file opener), like for ib_safe_file_access().
> Also, completely prevent user memory accesses from ->release().

Band-aid it is, and a bloody awful one, at that.  What the hell is going on
in bsg_put_device() and can it _ever_ hit that call chain?  I.e.
	bsg_release()
		bsg_put_device()
			blk_complete_sgv4_hdr_rq()
				->complete_rq()
					copy_to_user()
If it can, the whole thing is FUBAR by design - ->release() may bloody well
be called in a context that has no userspace at all.

This is completely insane; what's going on there?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 16:44   ` Jann Horn
@ 2018-06-15 16:53     ` Al Viro
  2018-06-15 17:10       ` Al Viro
  0 siblings, 1 reply; 23+ messages in thread
From: Al Viro @ 2018-06-15 16:53 UTC (permalink / raw)
  To: Jann Horn
  Cc: axboe, fujita.tomonori, dgilbert, jejb, martin.petersen,
	linux-block, linux-scsi, kernel list, Kernel Hardening, security

On Fri, Jun 15, 2018 at 06:44:51PM +0200, Jann Horn wrote:
> On Fri, Jun 15, 2018 at 6:40 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
> >
> > > I've mostly copypasted ib_safe_file_access() over as
> > > scsi_safe_file_access() because I couldn't find a good common header -
> > > please tell me if you know a better way.
> > > The duplicate pr_err_once() calls are so that each of them fires once;
> > > otherwise, this would probably have to be a macro.
> > >
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Jann Horn <jannh@google.com>
> > > ---
> >
> > WTF do you mean, in ->release()?  That makes no sense whatsoever -
> > what kind of copy_{to,from}_user() would be possible in there?
> 
> bsg_release -> bsg_put_device -> bsg_complete_all_commands ->
> blk_complete_sgv4_hdr_rq -> bsg_scsi_complete_rq -> copy_to_user.
> I don't think that was intentional.
> 
> Basically, the sense buffer is copied to a userspace address supplied
> in the previous ->write() when you ->read() the reply. But when you
> ->release() the file without reading the reply, they have to clean it
> up, and for that, they reuse the same code they use for ->read() - so
> the sense buffer is written to userspace on ->release().

Pardon me, that has only one fix - git rm.  This is too broken for words -
if your reading is correct, the interface is unsalvagable.  I hope you
*are* misreading it, but if not... how did that insanity get through
review at merge time?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 16:49 ` Al Viro
@ 2018-06-15 16:58   ` Jann Horn
  2018-06-15 17:02     ` Jann Horn
  0 siblings, 1 reply; 23+ messages in thread
From: Jann Horn @ 2018-06-15 16:58 UTC (permalink / raw)
  To: Al Viro
  Cc: axboe, fujita.tomonori, dgilbert, jejb, martin.petersen,
	linux-block, linux-scsi, kernel list, Kernel Hardening, security

On Fri, Jun 15, 2018 at 6:49 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
> > As Al Viro noted in commit 128394eff343 ("sg_write()/bsg_write() is not fit
> > to be called under KERNEL_DS"), sg and bsg improperly access userspace
> > memory outside the provided buffer, permitting kernel memory corruption via
> > splice().
> > But they don't just do it on ->write(), also on ->read() and (in the case
> > of bsg) even on ->release().
> >
> > As a band-aid, make sure that the ->read() and ->write() handlers can not
> > be called in weird contexts (kernel context or credentials different from
> > file opener), like for ib_safe_file_access().
> > Also, completely prevent user memory accesses from ->release().
>
> Band-aid it is, and a bloody awful one, at that.  What the hell is going on
> in bsg_put_device() and can it _ever_ hit that call chain?  I.e.
>         bsg_release()
>                 bsg_put_device()
>                         blk_complete_sgv4_hdr_rq()
>                                 ->complete_rq()
>                                         copy_to_user()
> If it can, the whole thing is FUBAR by design - ->release() may bloody well
> be called in a context that has no userspace at all.
>
> This is completely insane; what's going on there?

Perhaps I should have split my patch into two parts; it consists of
two somewhat related changes.

The first change is that ->read() and ->write() violate the normal
contract and, as a band-aid, should not be called in uaccess_kernel()
context or with changed creds.

The second change is an actual fix: AFAICS ->release() accidentally
accessed userspace, which I've fixed using the added "cleaning_up"
parameter.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 16:58   ` Jann Horn
@ 2018-06-15 17:02     ` Jann Horn
  0 siblings, 0 replies; 23+ messages in thread
From: Jann Horn @ 2018-06-15 17:02 UTC (permalink / raw)
  To: Al Viro
  Cc: axboe, fujita.tomonori, dgilbert, jejb, martin.petersen,
	linux-block, linux-scsi, kernel list, Kernel Hardening, security

On Fri, Jun 15, 2018 at 6:58 PM Jann Horn <jannh@google.com> wrote:
>
> On Fri, Jun 15, 2018 at 6:49 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
> > > As Al Viro noted in commit 128394eff343 ("sg_write()/bsg_write() is not fit
> > > to be called under KERNEL_DS"), sg and bsg improperly access userspace
> > > memory outside the provided buffer, permitting kernel memory corruption via
> > > splice().
> > > But they don't just do it on ->write(), also on ->read() and (in the case
> > > of bsg) even on ->release().
> > >
> > > As a band-aid, make sure that the ->read() and ->write() handlers can not
> > > be called in weird contexts (kernel context or credentials different from
> > > file opener), like for ib_safe_file_access().
> > > Also, completely prevent user memory accesses from ->release().
> >
> > Band-aid it is, and a bloody awful one, at that.  What the hell is going on
> > in bsg_put_device() and can it _ever_ hit that call chain?  I.e.
> >         bsg_release()
> >                 bsg_put_device()
> >                         blk_complete_sgv4_hdr_rq()
> >                                 ->complete_rq()
> >                                         copy_to_user()
> > If it can, the whole thing is FUBAR by design - ->release() may bloody well
> > be called in a context that has no userspace at all.
> >
> > This is completely insane; what's going on there?
>
> Perhaps I should have split my patch into two parts; it consists of
> two somewhat related changes.
>
> The first change is that ->read() and ->write() violate the normal
> contract and, as a band-aid, should not be called in uaccess_kernel()
> context or with changed creds.
>
> The second change is an actual fix: AFAICS ->release() accidentally
> accessed userspace, which I've fixed using the added "cleaning_up"
> parameter.

FWIW, the demo code I'm using to test this in a QEMU VM:

$ cat test.c
#define _GNU_SOURCE
#include <fcntl.h>
#include <unistd.h>
#include <linux/bsg.h>
#include <string.h>
#include <err.h>
#include <stdio.h>

int main(void) {
  int fd = open("/dev/bsg/0:0:0:0", O_RDWR);
  if (fd == -1)
    err(1, "foo");
  __u8 buf1[255];
  __u8 request[10] = {
    [0] = 0x5a, // MODE_SENSE_10
    [2] = 0x41,
    [8] = 0x10
  };
  __u8 sense[32];
  memset(sense, 'A', sizeof(sense));
  memset(buf1, 'A', sizeof(buf1));
  struct sg_io_v4 req = {
    .guard = 'Q',
    .protocol = BSG_PROTOCOL_SCSI,
    .subprotocol = BSG_SUB_PROTOCOL_SCSI_CMD,
    .request_len = sizeof(request),
    .request = (__u64)request,
    .max_response_len = sizeof(sense),
    .response = (__u64)sense,
    .din_xfer_len = sizeof(buf1),
    .din_xferp = (__u64)buf1,
    .timeout = 1000
  };
  if (write(fd, &req, sizeof(req)) != sizeof(req))
    err(1, "write");
  printf("sense[0] after write: 0x%02hhx\n", sense[0]);

  /*
  struct sg_io_v4 resp;
  if (splice(fd, NULL, pipe_fds[1], NULL, sizeof(struct sg_io_v4), 0)
!= sizeof(struct sg_io_v4))
    err(1, "splice");
    */

  sleep(1);
  printf("sense[0] after sleep: 0x%02hhx\n", sense[0]);
  close(fd);
  printf("sense[0] after close: 0x%02hhx\n", sense[0]);
}
$ gcc -o test test.c -Wall && sudo ./test
sense[0] after write: 0x41
sense[0] after sleep: 0x41
sense[0] after close: 0xf0
$ uname -a
Linux debian 4.17.0+ #10 SMP Fri Jun 15 14:48:42 CEST 2018 x86_64 GNU/Linux

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 16:53     ` Al Viro
@ 2018-06-15 17:10       ` Al Viro
  2018-06-15 17:13         ` Jann Horn
  0 siblings, 1 reply; 23+ messages in thread
From: Al Viro @ 2018-06-15 17:10 UTC (permalink / raw)
  To: Jann Horn
  Cc: axboe, fujita.tomonori, dgilbert, jejb, martin.petersen,
	linux-block, linux-scsi, kernel list, Kernel Hardening, security

On Fri, Jun 15, 2018 at 05:53:10PM +0100, Al Viro wrote:
> On Fri, Jun 15, 2018 at 06:44:51PM +0200, Jann Horn wrote:
> > On Fri, Jun 15, 2018 at 6:40 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
> > >
> > > > I've mostly copypasted ib_safe_file_access() over as
> > > > scsi_safe_file_access() because I couldn't find a good common header -
> > > > please tell me if you know a better way.
> > > > The duplicate pr_err_once() calls are so that each of them fires once;
> > > > otherwise, this would probably have to be a macro.
> > > >
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Cc: <stable@vger.kernel.org>
> > > > Signed-off-by: Jann Horn <jannh@google.com>
> > > > ---
> > >
> > > WTF do you mean, in ->release()?  That makes no sense whatsoever -
> > > what kind of copy_{to,from}_user() would be possible in there?
> > 
> > bsg_release -> bsg_put_device -> bsg_complete_all_commands ->
> > blk_complete_sgv4_hdr_rq -> bsg_scsi_complete_rq -> copy_to_user.
> > I don't think that was intentional.
> > 
> > Basically, the sense buffer is copied to a userspace address supplied
> > in the previous ->write() when you ->read() the reply. But when you
> > ->release() the file without reading the reply, they have to clean it
> > up, and for that, they reuse the same code they use for ->read() - so
> > the sense buffer is written to userspace on ->release().
> 
> Pardon me, that has only one fix - git rm.  This is too broken for words -
> if your reading is correct, the interface is unsalvagable.  I hope you
> *are* misreading it, but if not... how did that insanity get through
> review at merge time?

AFAICS, it went in as part of commit 3d6392cfbd7d "bsg: support for full
generic block layer SG v3", so your 2.6.12-rc2 is too old...

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 17:10       ` Al Viro
@ 2018-06-15 17:13         ` Jann Horn
  0 siblings, 0 replies; 23+ messages in thread
From: Jann Horn @ 2018-06-15 17:13 UTC (permalink / raw)
  To: Al Viro
  Cc: axboe, fujita.tomonori, dgilbert, jejb, martin.petersen,
	linux-block, linux-scsi, kernel list, Kernel Hardening, security

On Fri, Jun 15, 2018 at 7:10 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Fri, Jun 15, 2018 at 05:53:10PM +0100, Al Viro wrote:
> > On Fri, Jun 15, 2018 at 06:44:51PM +0200, Jann Horn wrote:
> > > On Fri, Jun 15, 2018 at 6:40 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > >
> > > > On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
> > > >
> > > > > I've mostly copypasted ib_safe_file_access() over as
> > > > > scsi_safe_file_access() because I couldn't find a good common header -
> > > > > please tell me if you know a better way.
> > > > > The duplicate pr_err_once() calls are so that each of them fires once;
> > > > > otherwise, this would probably have to be a macro.
> > > > >
> > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > > Cc: <stable@vger.kernel.org>
> > > > > Signed-off-by: Jann Horn <jannh@google.com>
> > > > > ---
> > > >
> > > > WTF do you mean, in ->release()?  That makes no sense whatsoever -
> > > > what kind of copy_{to,from}_user() would be possible in there?
> > >
> > > bsg_release -> bsg_put_device -> bsg_complete_all_commands ->
> > > blk_complete_sgv4_hdr_rq -> bsg_scsi_complete_rq -> copy_to_user.
> > > I don't think that was intentional.
> > >
> > > Basically, the sense buffer is copied to a userspace address supplied
> > > in the previous ->write() when you ->read() the reply. But when you
> > > ->release() the file without reading the reply, they have to clean it
> > > up, and for that, they reuse the same code they use for ->read() - so
> > > the sense buffer is written to userspace on ->release().
> >
> > Pardon me, that has only one fix - git rm.  This is too broken for words -
> > if your reading is correct, the interface is unsalvagable.  I hope you
> > *are* misreading it, but if not... how did that insanity get through
> > review at merge time?
>
> AFAICS, it went in as part of commit 3d6392cfbd7d "bsg: support for full
> generic block layer SG v3", so your 2.6.12-rc2 is too old...

I picked 2.6.12-rc2 for the Fixes tag because the bad copy_to_user()
in sg_new_read() is at least that old.
Do you think I should split this up into two patches or so - one for
the creds/uaccess_kernel checks, one for the ->release() bug?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 16:40 ` Al Viro
  2018-06-15 16:44   ` Jann Horn
@ 2018-06-15 20:47   ` Douglas Gilbert
  2018-06-18 15:26     ` Benjamin Block
  2018-06-18 15:37     ` Jens Axboe
  1 sibling, 2 replies; 23+ messages in thread
From: Douglas Gilbert @ 2018-06-15 20:47 UTC (permalink / raw)
  To: Al Viro, Jann Horn
  Cc: Jens Axboe, FUJITA Tomonori, James E.J. Bottomley,
	Martin K. Petersen, linux-block, linux-scsi, linux-kernel,
	kernel-hardening, security

On 2018-06-15 12:40 PM, Al Viro wrote:
> On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
> 
>> I've mostly copypasted ib_safe_file_access() over as
>> scsi_safe_file_access() because I couldn't find a good common header -
>> please tell me if you know a better way.
>> The duplicate pr_err_once() calls are so that each of them fires once;
>> otherwise, this would probably have to be a macro.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Jann Horn <jannh@google.com>
>> ---
> 
> WTF do you mean, in ->release()?  That makes no sense whatsoever -
> what kind of copy_{to,from}_user() would be possible in there?

The folks responsible are no longer active in kernel development ***
but as far as I know the async write(command), read(response) were
added to bsg over 10 years ago as proof-of-concept and never properly
worked in this async mode. The biggest design problem with it that I'm
aware of is that two tasks which issue write(commands) at about the
same time to the same device can receive one another's read(response).
The tracking of which response belongs to which task is in part the
reason why the sg driver's data structures are more complex than the
bsg driver's are. Hence the code work to fix that problem in the bsg
driver is not trivial and probably a reason why no-one has attempted it.

Once real world users (needing an async SCSI (or general storage)
pass-through) find out about that bsg "feature", they don't use it.
They use the sg driver or something else. So the fact that bsg has
other glaring errors in it in async mode is no surprise to me.

When I took over the maintenance of the sg driver in 1998, it only
had an async (i.e. write(command), read(response)) interface.
The SG_IO ioctl was added at the suggestion of Jørg Schilling (of
cdrecord "fame"). The sg driver implementation was essentially to
put a write(command) and read(response) back-to-back. The bsg driver
came along later and started with the synchronous SG_IO ioctl
interface only. The async write(command)/read(response) functionality
was added later to bsg. Perhaps that part of the bsg driver should be
deprecated/withdrawn if a maintainer/rewriter cannot be found.
[BTW the bsg sync SG_IO ioctl implementation can probably get the
wrong response, it's just that the window is a lot narrower.]

That said, the bsg driver has lots of other users. For example it is
the only generic pass-through in Linux for the SAS Management Protocol
(SMP) used to control SAS based storage enclosures. I have a user space
package based on it (in Linux) called smp_utils which works well IMO.
However disk enclosures won't typically have contention between users
trying to control them and I'm not aware of any disk enclosures that
support Persistent Reservations. So the bsg driver's "async" problems
are not a practical issue in this case. Also I believe some high end
storage hardware uses bsg to communicate with their hardware from their
user space tools.


Just some observations from an interested observer ...

Doug Gilbert


*** Well Jens Axbø's Copyright notice is on the bsg driver, together with
     and Peter M. Jones. Since I have been watching the bsg driver I'm
     not aware of any substantial patches or reworks for them. As far as
     I know FUJITA Tomonori did a ground up rewrite of it and he no longer
     works in this area. Makes you wonder what exactly Copyright banners
     mean on some code; 10, 15, 20 years on.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 20:47   ` Douglas Gilbert
@ 2018-06-18 15:26     ` Benjamin Block
  2018-06-18 15:37     ` Jens Axboe
  1 sibling, 0 replies; 23+ messages in thread
From: Benjamin Block @ 2018-06-18 15:26 UTC (permalink / raw)
  To: Douglas Gilbert
  Cc: Al Viro, Jann Horn, Jens Axboe, FUJITA Tomonori,
	James E.J. Bottomley, Martin K. Petersen, linux-block,
	linux-scsi, linux-kernel, kernel-hardening, security

On Fri, Jun 15, 2018 at 04:47:47PM -0400, Douglas Gilbert wrote:
> On 2018-06-15 12:40 PM, Al Viro wrote:
> > On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
> > 
> >> I've mostly copypasted ib_safe_file_access() over as
> >> scsi_safe_file_access() because I couldn't find a good common header -
> >> please tell me if you know a better way.
> >> The duplicate pr_err_once() calls are so that each of them fires once;
> >> otherwise, this would probably have to be a macro.
> >>
> >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> >> Cc: <stable@vger.kernel.org>
> >> Signed-off-by: Jann Horn <jannh@google.com>
> >> ---
> > 
> > WTF do you mean, in ->release()?  That makes no sense whatsoever -
> > what kind of copy_{to,from}_user() would be possible in there?
> 
> The folks responsible are no longer active in kernel development ***
> but as far as I know the async write(command), read(response) were
> added to bsg over 10 years ago as proof-of-concept and never properly
> worked in this async mode. The biggest design problem with it that I'm
> aware of is that two tasks which issue write(commands) at about the
> same time to the same device can receive one another's read(response).
> The tracking of which response belongs to which task is in part the
> reason why the sg driver's data structures are more complex than the
> bsg driver's are. Hence the code work to fix that problem in the bsg
> driver is not trivial and probably a reason why no-one has attempted it.
> 
> Once real world users (needing an async SCSI (or general storage)
> pass-through) find out about that bsg "feature", they don't use it.
> They use the sg driver or something else. So the fact that bsg has
> other glaring errors in it in async mode is no surprise to me.
> 
> When I took over the maintenance of the sg driver in 1998, it only
> had an async (i.e. write(command), read(response)) interface.
> The SG_IO ioctl was added at the suggestion of Jørg Schilling (of
> cdrecord "fame"). The sg driver implementation was essentially to
> put a write(command) and read(response) back-to-back. The bsg driver
> came along later and started with the synchronous SG_IO ioctl
> interface only. The async write(command)/read(response) functionality
> was added later to bsg. Perhaps that part of the bsg driver should be
> deprecated/withdrawn if a maintainer/rewriter cannot be found.
> [BTW the bsg sync SG_IO ioctl implementation can probably get the
> wrong response, it's just that the window is a lot narrower.]
> 
> That said, the bsg driver has lots of other users. For example it is
> the only generic pass-through in Linux for the SAS Management Protocol
> (SMP) used to control SAS based storage enclosures. I have a user space
> package based on it (in Linux) called smp_utils which works well IMO.
> However disk enclosures won't typically have contention between users
> trying to control them and I'm not aware of any disk enclosures that
> support Persistent Reservations. So the bsg driver's "async" problems
> are not a practical issue in this case. Also I believe some high end
> storage hardware uses bsg to communicate with their hardware from their
> user space tools.
> 

We definitely use the BSG IOCTL part for FC-Command passthrough (and
other SCSI transports need it for theirs too, SAS, iSCSI, ...). And I am
not aware of any other way to do that right now. So this can not go away
without someone giving us a different way to do that.

That said.. the read()/write() interface is just pointless atm, I don't
see any sane way of using that in userspace.

FWIW, I actually thought about rewriting that part so it becomes
somewhat sane (tracking which thread sends what command and so on), but
haven't had time to really doing it. With the whole discussion now, I am
not sure anyone really needs that anyway.


- Benjamin

--
With Best Regards, Benjamin Block      /      Linux on IBM Z Kernel Development
IBM Systems & Technology Group   /  IBM Deutschland Research & Development GmbH
Vorsitz. AufsR.: Martina Koederitz       /      Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 20:47   ` Douglas Gilbert
  2018-06-18 15:26     ` Benjamin Block
@ 2018-06-18 15:37     ` Jens Axboe
  2018-06-18 16:16       ` Al Viro
  2018-06-21 12:34       ` Christoph Hellwig
  1 sibling, 2 replies; 23+ messages in thread
From: Jens Axboe @ 2018-06-18 15:37 UTC (permalink / raw)
  To: dgilbert, Al Viro, Jann Horn
  Cc: FUJITA Tomonori, James E.J. Bottomley, Martin K. Petersen,
	linux-block, linux-scsi, linux-kernel, kernel-hardening,
	security

On 6/15/18 2:47 PM, Douglas Gilbert wrote:
> On 2018-06-15 12:40 PM, Al Viro wrote:
>> On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
>>
>>> I've mostly copypasted ib_safe_file_access() over as
>>> scsi_safe_file_access() because I couldn't find a good common header -
>>> please tell me if you know a better way.
>>> The duplicate pr_err_once() calls are so that each of them fires once;
>>> otherwise, this would probably have to be a macro.
>>>
>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Jann Horn <jannh@google.com>
>>> ---
>>
>> WTF do you mean, in ->release()?  That makes no sense whatsoever -
>> what kind of copy_{to,from}_user() would be possible in there?
> 
> The folks responsible are no longer active in kernel development ***
> but as far as I know the async write(command), read(response) were
> added to bsg over 10 years ago as proof-of-concept and never properly
> worked in this async mode. The biggest design problem with it that I'm

It was born with that mode, but I don't think anyone ever really used it.
So it might feasible to simply yank it. That said, just doing a prune
mode at ->release() time doesn't seem like such a hard task.

> aware of is that two tasks which issue write(commands) at about the
> same time to the same device can receive one another's read(response).
> The tracking of which response belongs to which task is in part the
> reason why the sg driver's data structures are more complex than the
> bsg driver's are. Hence the code work to fix that problem in the bsg
> driver is not trivial and probably a reason why no-one has attempted it.
> 
> Once real world users (needing an async SCSI (or general storage)
> pass-through) find out about that bsg "feature", they don't use it.
> They use the sg driver or something else. So the fact that bsg has
> other glaring errors in it in async mode is no surprise to me.
> 
> When I took over the maintenance of the sg driver in 1998, it only
> had an async (i.e. write(command), read(response)) interface.
> The SG_IO ioctl was added at the suggestion of Jørg Schilling (of
> cdrecord "fame"). The sg driver implementation was essentially to
> put a write(command) and read(response) back-to-back. The bsg driver
> came along later and started with the synchronous SG_IO ioctl
> interface only. The async write(command)/read(response) functionality
> was added later to bsg. Perhaps that part of the bsg driver should be
> deprecated/withdrawn if a maintainer/rewriter cannot be found.
> [BTW the bsg sync SG_IO ioctl implementation can probably get the
> wrong response, it's just that the window is a lot narrower.]

Feature wise, I don't think it ever changed. The read/write async
mode was included from the get-go.

> 
> That said, the bsg driver has lots of other users. For example it is
> the only generic pass-through in Linux for the SAS Management Protocol
> (SMP) used to control SAS based storage enclosures. I have a user space
> package based on it (in Linux) called smp_utils which works well IMO.
> However disk enclosures won't typically have contention between users
> trying to control them and I'm not aware of any disk enclosures that
> support Persistent Reservations. So the bsg driver's "async" problems
> are not a practical issue in this case. Also I believe some high end
> storage hardware uses bsg to communicate with their hardware from their
> user space tools.
> 
> 
> Just some observations from an interested observer ...
> 
> Doug Gilbert
> 
> 
> *** Well Jens Axbø's Copyright notice is on the bsg driver, together with
>      and Peter M. Jones. Since I have been watching the bsg driver I'm
>      not aware of any substantial patches or reworks for them. As far as
>      I know FUJITA Tomonori did a ground up rewrite of it and he no longer
>      works in this area. Makes you wonder what exactly Copyright banners
>      mean on some code; 10, 15, 20 years on.

It was never re-written. I handed it over to Fujita about 11 years ago,
but there was never any rewrite.

BTW, don't ever write my name like that, the 'oe' is not a spelled out
ascii variant, it's my name. For Jörg, it's o with umlaut, not the
Danish/Norwegian variant (he's German).

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-18 15:37     ` Jens Axboe
@ 2018-06-18 16:16       ` Al Viro
  2018-06-18 16:23         ` Jens Axboe
  2018-06-21 12:34       ` Christoph Hellwig
  1 sibling, 1 reply; 23+ messages in thread
From: Al Viro @ 2018-06-18 16:16 UTC (permalink / raw)
  To: Jens Axboe
  Cc: dgilbert, Jann Horn, FUJITA Tomonori, James E.J. Bottomley,
	Martin K. Petersen, linux-block, linux-scsi, linux-kernel,
	kernel-hardening, security

On Mon, Jun 18, 2018 at 09:37:01AM -0600, Jens Axboe wrote:

> > The folks responsible are no longer active in kernel development ***
> > but as far as I know the async write(command), read(response) were
> > added to bsg over 10 years ago as proof-of-concept and never properly
> > worked in this async mode. The biggest design problem with it that I'm
> 
> It was born with that mode, but I don't think anyone ever really used it.
> So it might feasible to simply yank it. That said, just doing a prune
> mode at ->release() time doesn't seem like such a hard task.

"prune mode" being...?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-18 16:16       ` Al Viro
@ 2018-06-18 16:23         ` Jens Axboe
  0 siblings, 0 replies; 23+ messages in thread
From: Jens Axboe @ 2018-06-18 16:23 UTC (permalink / raw)
  To: Al Viro
  Cc: dgilbert, Jann Horn, FUJITA Tomonori, James E.J. Bottomley,
	Martin K. Petersen, linux-block, linux-scsi, linux-kernel,
	kernel-hardening, security

On 6/18/18 10:16 AM, Al Viro wrote:
> On Mon, Jun 18, 2018 at 09:37:01AM -0600, Jens Axboe wrote:
> 
>>> The folks responsible are no longer active in kernel development ***
>>> but as far as I know the async write(command), read(response) were
>>> added to bsg over 10 years ago as proof-of-concept and never properly
>>> worked in this async mode. The biggest design problem with it that I'm
>>
>> It was born with that mode, but I don't think anyone ever really used it.
>> So it might feasible to simply yank it. That said, just doing a prune
>> mode at ->release() time doesn't seem like such a hard task.
> 
> "prune mode" being...?

Basically what Jann posted, not doing any copy-back of data. Need to
verify if the bio unmapping is handled correctly, as some of those
will also copy when the end_io handling is invoked.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-18 15:37     ` Jens Axboe
  2018-06-18 16:16       ` Al Viro
@ 2018-06-21 12:34       ` Christoph Hellwig
  2018-06-21 12:51         ` Jann Horn
  2018-06-21 14:07         ` Jens Axboe
  1 sibling, 2 replies; 23+ messages in thread
From: Christoph Hellwig @ 2018-06-21 12:34 UTC (permalink / raw)
  To: Jens Axboe
  Cc: dgilbert, Al Viro, Jann Horn, FUJITA Tomonori,
	James E.J. Bottomley, Martin K. Petersen, linux-block,
	linux-scsi, linux-kernel, kernel-hardening, security

On Mon, Jun 18, 2018 at 09:37:01AM -0600, Jens Axboe wrote:
> It was born with that mode, but I don't think anyone ever really used it.
> So it might feasible to simply yank it. That said, just doing a prune
> mode at ->release() time doesn't seem like such a hard task.

Let's try to kill it.  It is a significant amount of code, which does
fishy things and is probably entirely unused:

---
From baec733be1b400d73d0fa2bfc07684598c4172e7 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 21 Jun 2018 14:31:32 +0200
Subject: bsg: remove read/write support

The code poses a security risk due to user memory access in ->release
and had an API that can't be used reliably.  As far as we know it was
never used for real, but if that turns out wrong we'll have to revert
this commit and come up with a band aid.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bsg.c | 460 +---------------------------------------------------
 1 file changed, 6 insertions(+), 454 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 66602c489956..0d2e9bf6208b 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -13,11 +13,9 @@
 #include <linux/init.h>
 #include <linux/file.h>
 #include <linux/blkdev.h>
-#include <linux/poll.h>
 #include <linux/cdev.h>
 #include <linux/jiffies.h>
 #include <linux/percpu.h>
-#include <linux/uio.h>
 #include <linux/idr.h>
 #include <linux/bsg.h>
 #include <linux/slab.h>
@@ -38,21 +36,10 @@
 struct bsg_device {
 	struct request_queue *queue;
 	spinlock_t lock;
-	struct list_head busy_list;
-	struct list_head done_list;
 	struct hlist_node dev_list;
 	atomic_t ref_count;
-	int queued_cmds;
-	int done_cmds;
-	wait_queue_head_t wq_done;
-	wait_queue_head_t wq_free;
 	char name[20];
 	int max_queue;
-	unsigned long flags;
-};
-
-enum {
-	BSG_F_BLOCK		= 1,
 };
 
 #define BSG_DEFAULT_CMDS	64
@@ -67,64 +54,6 @@ static struct hlist_head bsg_device_list[BSG_LIST_ARRAY_SIZE];
 static struct class *bsg_class;
 static int bsg_major;
 
-static struct kmem_cache *bsg_cmd_cachep;
-
-/*
- * our internal command type
- */
-struct bsg_command {
-	struct bsg_device *bd;
-	struct list_head list;
-	struct request *rq;
-	struct bio *bio;
-	struct bio *bidi_bio;
-	int err;
-	struct sg_io_v4 hdr;
-};
-
-static void bsg_free_command(struct bsg_command *bc)
-{
-	struct bsg_device *bd = bc->bd;
-	unsigned long flags;
-
-	kmem_cache_free(bsg_cmd_cachep, bc);
-
-	spin_lock_irqsave(&bd->lock, flags);
-	bd->queued_cmds--;
-	spin_unlock_irqrestore(&bd->lock, flags);
-
-	wake_up(&bd->wq_free);
-}
-
-static struct bsg_command *bsg_alloc_command(struct bsg_device *bd)
-{
-	struct bsg_command *bc = ERR_PTR(-EINVAL);
-
-	spin_lock_irq(&bd->lock);
-
-	if (bd->queued_cmds >= bd->max_queue)
-		goto out;
-
-	bd->queued_cmds++;
-	spin_unlock_irq(&bd->lock);
-
-	bc = kmem_cache_zalloc(bsg_cmd_cachep, GFP_KERNEL);
-	if (unlikely(!bc)) {
-		spin_lock_irq(&bd->lock);
-		bd->queued_cmds--;
-		bc = ERR_PTR(-ENOMEM);
-		goto out;
-	}
-
-	bc->bd = bd;
-	INIT_LIST_HEAD(&bc->list);
-	bsg_dbg(bd, "returning free cmd %p\n", bc);
-	return bc;
-out:
-	spin_unlock_irq(&bd->lock);
-	return bc;
-}
-
 static inline struct hlist_head *bsg_dev_idx_hash(int index)
 {
 	return &bsg_device_list[index & (BSG_LIST_ARRAY_SIZE - 1)];
@@ -287,101 +216,6 @@ bsg_map_hdr(struct request_queue *q, struct sg_io_v4 *hdr, fmode_t mode)
 	return ERR_PTR(ret);
 }
 
-/*
- * async completion call-back from the block layer, when scsi/ide/whatever
- * calls end_that_request_last() on a request
- */
-static void bsg_rq_end_io(struct request *rq, blk_status_t status)
-{
-	struct bsg_command *bc = rq->end_io_data;
-	struct bsg_device *bd = bc->bd;
-	unsigned long flags;
-
-	bsg_dbg(bd, "finished rq %p bc %p, bio %p\n",
-		rq, bc, bc->bio);
-
-	bc->hdr.duration = jiffies_to_msecs(jiffies - bc->hdr.duration);
-
-	spin_lock_irqsave(&bd->lock, flags);
-	list_move_tail(&bc->list, &bd->done_list);
-	bd->done_cmds++;
-	spin_unlock_irqrestore(&bd->lock, flags);
-
-	wake_up(&bd->wq_done);
-}
-
-/*
- * do final setup of a 'bc' and submit the matching 'rq' to the block
- * layer for io
- */
-static void bsg_add_command(struct bsg_device *bd, struct request_queue *q,
-			    struct bsg_command *bc, struct request *rq)
-{
-	int at_head = (0 == (bc->hdr.flags & BSG_FLAG_Q_AT_TAIL));
-
-	/*
-	 * add bc command to busy queue and submit rq for io
-	 */
-	bc->rq = rq;
-	bc->bio = rq->bio;
-	if (rq->next_rq)
-		bc->bidi_bio = rq->next_rq->bio;
-	bc->hdr.duration = jiffies;
-	spin_lock_irq(&bd->lock);
-	list_add_tail(&bc->list, &bd->busy_list);
-	spin_unlock_irq(&bd->lock);
-
-	bsg_dbg(bd, "queueing rq %p, bc %p\n", rq, bc);
-
-	rq->end_io_data = bc;
-	blk_execute_rq_nowait(q, NULL, rq, at_head, bsg_rq_end_io);
-}
-
-static struct bsg_command *bsg_next_done_cmd(struct bsg_device *bd)
-{
-	struct bsg_command *bc = NULL;
-
-	spin_lock_irq(&bd->lock);
-	if (bd->done_cmds) {
-		bc = list_first_entry(&bd->done_list, struct bsg_command, list);
-		list_del(&bc->list);
-		bd->done_cmds--;
-	}
-	spin_unlock_irq(&bd->lock);
-
-	return bc;
-}
-
-/*
- * Get a finished command from the done list
- */
-static struct bsg_command *bsg_get_done_cmd(struct bsg_device *bd)
-{
-	struct bsg_command *bc;
-	int ret;
-
-	do {
-		bc = bsg_next_done_cmd(bd);
-		if (bc)
-			break;
-
-		if (!test_bit(BSG_F_BLOCK, &bd->flags)) {
-			bc = ERR_PTR(-EAGAIN);
-			break;
-		}
-
-		ret = wait_event_interruptible(bd->wq_done, bd->done_cmds);
-		if (ret) {
-			bc = ERR_PTR(-ERESTARTSYS);
-			break;
-		}
-	} while (1);
-
-	bsg_dbg(bd, "returning done %p\n", bc);
-
-	return bc;
-}
-
 static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 				    struct bio *bio, struct bio *bidi_bio)
 {
@@ -400,234 +234,6 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 	return ret;
 }
 
-static bool bsg_complete(struct bsg_device *bd)
-{
-	bool ret = false;
-	bool spin;
-
-	do {
-		spin_lock_irq(&bd->lock);
-
-		BUG_ON(bd->done_cmds > bd->queued_cmds);
-
-		/*
-		 * All commands consumed.
-		 */
-		if (bd->done_cmds == bd->queued_cmds)
-			ret = true;
-
-		spin = !test_bit(BSG_F_BLOCK, &bd->flags);
-
-		spin_unlock_irq(&bd->lock);
-	} while (!ret && spin);
-
-	return ret;
-}
-
-static int bsg_complete_all_commands(struct bsg_device *bd)
-{
-	struct bsg_command *bc;
-	int ret, tret;
-
-	bsg_dbg(bd, "entered\n");
-
-	/*
-	 * wait for all commands to complete
-	 */
-	io_wait_event(bd->wq_done, bsg_complete(bd));
-
-	/*
-	 * discard done commands
-	 */
-	ret = 0;
-	do {
-		spin_lock_irq(&bd->lock);
-		if (!bd->queued_cmds) {
-			spin_unlock_irq(&bd->lock);
-			break;
-		}
-		spin_unlock_irq(&bd->lock);
-
-		bc = bsg_get_done_cmd(bd);
-		if (IS_ERR(bc))
-			break;
-
-		tret = blk_complete_sgv4_hdr_rq(bc->rq, &bc->hdr, bc->bio,
-						bc->bidi_bio);
-		if (!ret)
-			ret = tret;
-
-		bsg_free_command(bc);
-	} while (1);
-
-	return ret;
-}
-
-static int
-__bsg_read(char __user *buf, size_t count, struct bsg_device *bd,
-	   const struct iovec *iov, ssize_t *bytes_read)
-{
-	struct bsg_command *bc;
-	int nr_commands, ret;
-
-	if (count % sizeof(struct sg_io_v4))
-		return -EINVAL;
-
-	ret = 0;
-	nr_commands = count / sizeof(struct sg_io_v4);
-	while (nr_commands) {
-		bc = bsg_get_done_cmd(bd);
-		if (IS_ERR(bc)) {
-			ret = PTR_ERR(bc);
-			break;
-		}
-
-		/*
-		 * this is the only case where we need to copy data back
-		 * after completing the request. so do that here,
-		 * bsg_complete_work() cannot do that for us
-		 */
-		ret = blk_complete_sgv4_hdr_rq(bc->rq, &bc->hdr, bc->bio,
-					       bc->bidi_bio);
-
-		if (copy_to_user(buf, &bc->hdr, sizeof(bc->hdr)))
-			ret = -EFAULT;
-
-		bsg_free_command(bc);
-
-		if (ret)
-			break;
-
-		buf += sizeof(struct sg_io_v4);
-		*bytes_read += sizeof(struct sg_io_v4);
-		nr_commands--;
-	}
-
-	return ret;
-}
-
-static inline void bsg_set_block(struct bsg_device *bd, struct file *file)
-{
-	if (file->f_flags & O_NONBLOCK)
-		clear_bit(BSG_F_BLOCK, &bd->flags);
-	else
-		set_bit(BSG_F_BLOCK, &bd->flags);
-}
-
-/*
- * Check if the error is a "real" error that we should return.
- */
-static inline int err_block_err(int ret)
-{
-	if (ret && ret != -ENOSPC && ret != -ENODATA && ret != -EAGAIN)
-		return 1;
-
-	return 0;
-}
-
-static ssize_t
-bsg_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
-{
-	struct bsg_device *bd = file->private_data;
-	int ret;
-	ssize_t bytes_read;
-
-	bsg_dbg(bd, "read %zd bytes\n", count);
-
-	bsg_set_block(bd, file);
-
-	bytes_read = 0;
-	ret = __bsg_read(buf, count, bd, NULL, &bytes_read);
-	*ppos = bytes_read;
-
-	if (!bytes_read || err_block_err(ret))
-		bytes_read = ret;
-
-	return bytes_read;
-}
-
-static int __bsg_write(struct bsg_device *bd, const char __user *buf,
-		       size_t count, ssize_t *bytes_written, fmode_t mode)
-{
-	struct bsg_command *bc;
-	struct request *rq;
-	int ret, nr_commands;
-
-	if (count % sizeof(struct sg_io_v4))
-		return -EINVAL;
-
-	nr_commands = count / sizeof(struct sg_io_v4);
-	rq = NULL;
-	bc = NULL;
-	ret = 0;
-	while (nr_commands) {
-		struct request_queue *q = bd->queue;
-
-		bc = bsg_alloc_command(bd);
-		if (IS_ERR(bc)) {
-			ret = PTR_ERR(bc);
-			bc = NULL;
-			break;
-		}
-
-		if (copy_from_user(&bc->hdr, buf, sizeof(bc->hdr))) {
-			ret = -EFAULT;
-			break;
-		}
-
-		/*
-		 * get a request, fill in the blanks, and add to request queue
-		 */
-		rq = bsg_map_hdr(bd->queue, &bc->hdr, mode);
-		if (IS_ERR(rq)) {
-			ret = PTR_ERR(rq);
-			rq = NULL;
-			break;
-		}
-
-		bsg_add_command(bd, q, bc, rq);
-		bc = NULL;
-		rq = NULL;
-		nr_commands--;
-		buf += sizeof(struct sg_io_v4);
-		*bytes_written += sizeof(struct sg_io_v4);
-	}
-
-	if (bc)
-		bsg_free_command(bc);
-
-	return ret;
-}
-
-static ssize_t
-bsg_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
-{
-	struct bsg_device *bd = file->private_data;
-	ssize_t bytes_written;
-	int ret;
-
-	bsg_dbg(bd, "write %zd bytes\n", count);
-
-	if (unlikely(uaccess_kernel()))
-		return -EINVAL;
-
-	bsg_set_block(bd, file);
-
-	bytes_written = 0;
-	ret = __bsg_write(bd, buf, count, &bytes_written, file->f_mode);
-
-	*ppos = bytes_written;
-
-	/*
-	 * return bytes written on non-fatal errors
-	 */
-	if (!bytes_written || err_block_err(ret))
-		bytes_written = ret;
-
-	bsg_dbg(bd, "returning %zd\n", bytes_written);
-	return bytes_written;
-}
-
 static struct bsg_device *bsg_alloc_device(void)
 {
 	struct bsg_device *bd;
@@ -637,29 +243,20 @@ static struct bsg_device *bsg_alloc_device(void)
 		return NULL;
 
 	spin_lock_init(&bd->lock);
-
 	bd->max_queue = BSG_DEFAULT_CMDS;
-
-	INIT_LIST_HEAD(&bd->busy_list);
-	INIT_LIST_HEAD(&bd->done_list);
 	INIT_HLIST_NODE(&bd->dev_list);
-
-	init_waitqueue_head(&bd->wq_free);
-	init_waitqueue_head(&bd->wq_done);
 	return bd;
 }
 
 static int bsg_put_device(struct bsg_device *bd)
 {
-	int ret = 0, do_free;
 	struct request_queue *q = bd->queue;
 
 	mutex_lock(&bsg_mutex);
 
-	do_free = atomic_dec_and_test(&bd->ref_count);
-	if (!do_free) {
+	if (!atomic_dec_and_test(&bd->ref_count)) {
 		mutex_unlock(&bsg_mutex);
-		goto out;
+		return 0;
 	}
 
 	hlist_del(&bd->dev_list);
@@ -670,20 +267,9 @@ static int bsg_put_device(struct bsg_device *bd)
 	/*
 	 * close can always block
 	 */
-	set_bit(BSG_F_BLOCK, &bd->flags);
-
-	/*
-	 * correct error detection baddies here again. it's the responsibility
-	 * of the app to properly reap commands before close() if it wants
-	 * fool-proof error detection
-	 */
-	ret = bsg_complete_all_commands(bd);
-
 	kfree(bd);
-out:
-	if (do_free)
-		blk_put_queue(q);
-	return ret;
+	blk_put_queue(q);
+	return 0;
 }
 
 static struct bsg_device *bsg_add_device(struct inode *inode,
@@ -706,8 +292,6 @@ static struct bsg_device *bsg_add_device(struct inode *inode,
 
 	bd->queue = rq;
 
-	bsg_set_block(bd, file);
-
 	atomic_set(&bd->ref_count, 1);
 	hlist_add_head(&bd->dev_list, bsg_dev_idx_hash(iminor(inode)));
 
@@ -781,24 +365,6 @@ static int bsg_release(struct inode *inode, struct file *file)
 	return bsg_put_device(bd);
 }
 
-static __poll_t bsg_poll(struct file *file, poll_table *wait)
-{
-	struct bsg_device *bd = file->private_data;
-	__poll_t mask = 0;
-
-	poll_wait(file, &bd->wq_done, wait);
-	poll_wait(file, &bd->wq_free, wait);
-
-	spin_lock_irq(&bd->lock);
-	if (!list_empty(&bd->done_list))
-		mask |= EPOLLIN | EPOLLRDNORM;
-	if (bd->queued_cmds < bd->max_queue)
-		mask |= EPOLLOUT;
-	spin_unlock_irq(&bd->lock);
-
-	return mask;
-}
-
 static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct bsg_device *bd = file->private_data;
@@ -872,9 +438,6 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 }
 
 static const struct file_operations bsg_fops = {
-	.read		=	bsg_read,
-	.write		=	bsg_write,
-	.poll		=	bsg_poll,
 	.open		=	bsg_open,
 	.release	=	bsg_release,
 	.unlocked_ioctl	=	bsg_ioctl,
@@ -979,21 +542,12 @@ static int __init bsg_init(void)
 	int ret, i;
 	dev_t devid;
 
-	bsg_cmd_cachep = kmem_cache_create("bsg_cmd",
-				sizeof(struct bsg_command), 0, 0, NULL);
-	if (!bsg_cmd_cachep) {
-		printk(KERN_ERR "bsg: failed creating slab cache\n");
-		return -ENOMEM;
-	}
-
 	for (i = 0; i < BSG_LIST_ARRAY_SIZE; i++)
 		INIT_HLIST_HEAD(&bsg_device_list[i]);
 
 	bsg_class = class_create(THIS_MODULE, "bsg");
-	if (IS_ERR(bsg_class)) {
-		ret = PTR_ERR(bsg_class);
-		goto destroy_kmemcache;
-	}
+	if (IS_ERR(bsg_class))
+		return PTR_ERR(bsg_class);
 	bsg_class->devnode = bsg_devnode;
 
 	ret = alloc_chrdev_region(&devid, 0, BSG_MAX_DEVS, "bsg");
@@ -1014,8 +568,6 @@ static int __init bsg_init(void)
 	unregister_chrdev_region(MKDEV(bsg_major, 0), BSG_MAX_DEVS);
 destroy_bsg_class:
 	class_destroy(bsg_class);
-destroy_kmemcache:
-	kmem_cache_destroy(bsg_cmd_cachep);
 	return ret;
 }
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-15 15:23 [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release Jann Horn
  2018-06-15 16:40 ` Al Viro
  2018-06-15 16:49 ` Al Viro
@ 2018-06-21 12:40 ` Christoph Hellwig
  2018-06-21 12:54   ` Jann Horn
  2 siblings, 1 reply; 23+ messages in thread
From: Christoph Hellwig @ 2018-06-21 12:40 UTC (permalink / raw)
  To: Jann Horn
  Cc: Jens Axboe, FUJITA Tomonori, Doug Gilbert, James E.J. Bottomley,
	Martin K. Petersen, linux-block, linux-scsi, linux-kernel,
	Al Viro, kernel-hardening, security

Can you resend a patch for the sg driver alone?  Also I think
we just want the scsi_safe_file_access code inside sg itself,
it really has nothing to do with the reset of the contents in
scsi_cmnd.h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-21 12:34       ` Christoph Hellwig
@ 2018-06-21 12:51         ` Jann Horn
  2018-06-21 13:03           ` Christoph Hellwig
  2018-06-21 14:07         ` Jens Axboe
  1 sibling, 1 reply; 23+ messages in thread
From: Jann Horn @ 2018-06-21 12:51 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, dgilbert, Al Viro, fujita.tomonori, jejb, martin.petersen,
	linux-block, linux-scsi, kernel list, Kernel Hardening, security

On Thu, Jun 21, 2018 at 2:34 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jun 18, 2018 at 09:37:01AM -0600, Jens Axboe wrote:
> > It was born with that mode, but I don't think anyone ever really used it.
> > So it might feasible to simply yank it. That said, just doing a prune
> > mode at ->release() time doesn't seem like such a hard task.
>
> Let's try to kill it.  It is a significant amount of code, which does
> fishy things and is probably entirely unused:
>
> ---
> From baec733be1b400d73d0fa2bfc07684598c4172e7 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 21 Jun 2018 14:31:32 +0200
> Subject: bsg: remove read/write support
>
> The code poses a security risk due to user memory access in ->release
> and had an API that can't be used reliably.  As far as we know it was
> never used for real, but if that turns out wrong we'll have to revert
> this commit and come up with a band aid.

FWIW, I just had a look through Debian's codesearch (which AFAIK scans
through the source code of all software that Debian packages) for uses
of struct sg_io_v4: https://codesearch.debian.net/search?q=sg_io_v4

Hits that seem to be using read() or write() with struct sg_io_v4 on
bsg devices:

In the package https://packages.debian.org/stretch/tgt:
  https://sources.debian.org/src/tgt/1:1.0.73-1/usr/bs_sg.c/?hl=131#L131
  https://sources.debian.org/src/tgt/1:1.0.73-1/usr/bs_sg.c/?hl=236#L236
In the package https://packages.debian.org/stretch/sg3-utils:
  https://sources.debian.org/src/sg3-utils/1.42-2/examples/bsg_queue_tst.c/?hl=60#L60

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-21 12:40 ` Christoph Hellwig
@ 2018-06-21 12:54   ` Jann Horn
  0 siblings, 0 replies; 23+ messages in thread
From: Jann Horn @ 2018-06-21 12:54 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, fujita.tomonori, dgilbert, jejb, martin.petersen,
	linux-block, linux-scsi, kernel list, Al Viro, Kernel Hardening,
	security

On Thu, Jun 21, 2018 at 2:40 PM Christoph Hellwig <hch@infradead.org> wrote:
> Can you resend a patch for the sg driver alone?

Okay, will do.

> Also I think
> we just want the scsi_safe_file_access code inside sg itself,
> it really has nothing to do with the reset of the contents in
> scsi_cmnd.h

Okay. (I put it there because I couldn't figure out a better common
header and didn't want to create two new copies of the code.)

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-21 12:51         ` Jann Horn
@ 2018-06-21 13:03           ` Christoph Hellwig
  0 siblings, 0 replies; 23+ messages in thread
From: Christoph Hellwig @ 2018-06-21 13:03 UTC (permalink / raw)
  To: Jann Horn
  Cc: Christoph Hellwig, axboe, dgilbert, Al Viro, fujita.tomonori,
	jejb, martin.petersen, linux-block, linux-scsi, kernel list,
	Kernel Hardening, security, Alexander Nezhinsky,
	Nicholas Bellinger, stgt

On Thu, Jun 21, 2018 at 02:51:16PM +0200, Jann Horn wrote:
> In the package https://packages.debian.org/stretch/tgt:
>   https://sources.debian.org/src/tgt/1:1.0.73-1/usr/bs_sg.c/?hl=131#L131
>   https://sources.debian.org/src/tgt/1:1.0.73-1/usr/bs_sg.c/?hl=236#L236

This is for real, although it is an optional case in an optional plug
in, so I'm not sure if any one actually uses bsg read/write with it
for real.

The usual aspects added. (the question is if bsg read/write support
is actually used in real life or can be removed)

> In the package https://packages.debian.org/stretch/sg3-utils:
>   https://sources.debian.org/src/sg3-utils/1.42-2/examples/bsg_queue_tst.c/?hl=60#L60

This is just example code.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-21 12:34       ` Christoph Hellwig
  2018-06-21 12:51         ` Jann Horn
@ 2018-06-21 14:07         ` Jens Axboe
  2018-07-08 14:58           ` Christoph Hellwig
  1 sibling, 1 reply; 23+ messages in thread
From: Jens Axboe @ 2018-06-21 14:07 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: dgilbert, Al Viro, Jann Horn, FUJITA Tomonori,
	James E.J. Bottomley, Martin K. Petersen, linux-block,
	linux-scsi, linux-kernel, kernel-hardening, security

On 6/21/18 6:34 AM, Christoph Hellwig wrote:
> On Mon, Jun 18, 2018 at 09:37:01AM -0600, Jens Axboe wrote:
>> It was born with that mode, but I don't think anyone ever really used it.
>> So it might feasible to simply yank it. That said, just doing a prune
>> mode at ->release() time doesn't seem like such a hard task.
> 
> Let's try to kill it.  It is a significant amount of code, which does
> fishy things and is probably entirely unused:

I'd be fine with that, if we knew that nobody uses it. But that's
really hard to figure out. I did see Jann's source code scan, which
even if non-exhaustive, still shows at least one user of it.

How about we just make the write interface sync? Then any copy can
happen while the we block the task, and the read side is just
copying the header info back, or dumping it if the task didn't
read it before it went away.

That will still be functional, just not queueable. But that's not
a huge concern, it won't break any applications. And with pure
sync issue, most of the code goes away anyway and becomes similar
to the sync ioctl.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-06-21 14:07         ` Jens Axboe
@ 2018-07-08 14:58           ` Christoph Hellwig
  2018-07-10 20:53             ` Jann Horn
  0 siblings, 1 reply; 23+ messages in thread
From: Christoph Hellwig @ 2018-07-08 14:58 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Christoph Hellwig, dgilbert, Al Viro, Jann Horn, FUJITA Tomonori,
	James E.J. Bottomley, Martin K. Petersen, linux-block,
	linux-scsi, linux-kernel, kernel-hardening, security

On Thu, Jun 21, 2018 at 08:07:23AM -0600, Jens Axboe wrote:
> I'd be fine with that, if we knew that nobody uses it. But that's
> really hard to figure out. I did see Jann's source code scan, which
> even if non-exhaustive, still shows at least one user of it.

One is an example, and the other looks very close to an example,
as far as I can tell it was Nic doing a bsg read/write WIP for a
tgt module without anyone every picking up on it.  I did add the tgt
list to Cc and no one seemed to care about the bsg read/write support.
Adding the tgt list back, but I doubt anyone ever actually used it.

> How about we just make the write interface sync? Then any copy can
> happen while the we block the task, and the read side is just
> copying the header info back, or dumping it if the task didn't
> read it before it went away.

How is that going to work?  As far as I can tell each I/O using
bsg read/write needs a write and a read, so they need to pair
and thus can't be a purely sync interface.

It also doesn't help with the issue that bsg_write may possible
write to user memory, which is highly unusal and asking for security
issues itself.

Either way, we should probably at very least apply a respun version
of the patch from Jann to 4.18-rc and -stable while we keep discussing
this.

Jann, can you respin the bsg patch with the same changes as the now
included sg one?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-07-08 14:58           ` Christoph Hellwig
@ 2018-07-10 20:53             ` Jann Horn
  2018-07-11  6:33               ` Christoph Hellwig
  0 siblings, 1 reply; 23+ messages in thread
From: Jann Horn @ 2018-07-10 20:53 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, Douglas Gilbert, Al Viro, fujita.tomonori, jejb,
	martin.petersen, linux-block, linux-scsi, kernel list,
	Kernel Hardening, security

On Sun, Jul 8, 2018 at 7:58 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Jun 21, 2018 at 08:07:23AM -0600, Jens Axboe wrote:
> > I'd be fine with that, if we knew that nobody uses it. But that's
> > really hard to figure out. I did see Jann's source code scan, which
> > even if non-exhaustive, still shows at least one user of it.
>
> One is an example, and the other looks very close to an example,
> as far as I can tell it was Nic doing a bsg read/write WIP for a
> tgt module without anyone every picking up on it.  I did add the tgt
> list to Cc and no one seemed to care about the bsg read/write support.
> Adding the tgt list back, but I doubt anyone ever actually used it.
>
> > How about we just make the write interface sync? Then any copy can
> > happen while the we block the task, and the read side is just
> > copying the header info back, or dumping it if the task didn't
> > read it before it went away.
>
> How is that going to work?  As far as I can tell each I/O using
> bsg read/write needs a write and a read, so they need to pair
> and thus can't be a purely sync interface.
>
> It also doesn't help with the issue that bsg_write may possible
> write to user memory, which is highly unusal and asking for security
> issues itself.
>
> Either way, we should probably at very least apply a respun version
> of the patch from Jann to 4.18-rc and -stable while we keep discussing
> this.
>
> Jann, can you respin the bsg patch with the same changes as the now
> included sg one?

With the error messages like in my sg patch or like with Linus'
proposed patch (https://lore.kernel.org/lkml/CA+55aFwg-2GP4ASTdd1pusmZkF7c8AN9febVDCaioDxzYJSLfw@mail.gmail.com/)
applied?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release
  2018-07-10 20:53             ` Jann Horn
@ 2018-07-11  6:33               ` Christoph Hellwig
  0 siblings, 0 replies; 23+ messages in thread
From: Christoph Hellwig @ 2018-07-11  6:33 UTC (permalink / raw)
  To: Jann Horn
  Cc: Christoph Hellwig, axboe, Douglas Gilbert, Al Viro,
	fujita.tomonori, jejb, martin.petersen, linux-block, linux-scsi,
	kernel list, Kernel Hardening, security

On Tue, Jul 10, 2018 at 01:53:02PM -0700, Jann Horn wrote:
> With the error messages like in my sg patch or like with Linus'
> proposed patch (https://lore.kernel.org/lkml/CA+55aFwg-2GP4ASTdd1pusmZkF7c8AN9febVDCaioDxzYJSLfw@mail.gmail.com/)
> applied?

I guess we should follows Linus' suggestions.

Note that he also seems to agree with me to just remove the read/write
interface.  Compared to sg bsg really is unused for the read/write
interface.  But lets get your fix in first..

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, back to index

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-15 15:23 [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release Jann Horn
2018-06-15 16:40 ` Al Viro
2018-06-15 16:44   ` Jann Horn
2018-06-15 16:53     ` Al Viro
2018-06-15 17:10       ` Al Viro
2018-06-15 17:13         ` Jann Horn
2018-06-15 20:47   ` Douglas Gilbert
2018-06-18 15:26     ` Benjamin Block
2018-06-18 15:37     ` Jens Axboe
2018-06-18 16:16       ` Al Viro
2018-06-18 16:23         ` Jens Axboe
2018-06-21 12:34       ` Christoph Hellwig
2018-06-21 12:51         ` Jann Horn
2018-06-21 13:03           ` Christoph Hellwig
2018-06-21 14:07         ` Jens Axboe
2018-07-08 14:58           ` Christoph Hellwig
2018-07-10 20:53             ` Jann Horn
2018-07-11  6:33               ` Christoph Hellwig
2018-06-15 16:49 ` Al Viro
2018-06-15 16:58   ` Jann Horn
2018-06-15 17:02     ` Jann Horn
2018-06-21 12:40 ` Christoph Hellwig
2018-06-21 12:54   ` Jann Horn

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox