scsi: sg: fix v3 compat read/write interface

Message ID 20191204140812.2761761-1-arnd@arndb.de
State Superseded
Headers show
Series
  • scsi: sg: fix v3 compat read/write interface
Related show

Commit Message

Arnd Bergmann Dec. 4, 2019, 2:08 p.m.
In the v5.4 merge window, a cleanup patch from Al Viro conflicted
with my rework of the compat handling for sg.c read(). Linus Torvalds
did a correct merge but pointed out that the resulting code is still
unsatisfactory.

I later noticed that the sg_new_read() function still gets the compat
mode wrong, when the 'count' argument is large enough to pass a
compat_sg_io_hdr object, but not a nativ sg_io_hdr.

To address both of these, move the definition of compat_sg_io_hdr
into a scsi/sg.h to make it visible to sg.c and rewrite the logic
for reading req_pack_id as well as the size check to a simpler
version that gets the expected results.

Fixes: c35a5cfb4150 ("scsi: sg: sg_read(): simplify reading ->pack_id of userland sg_io_hdr_t")
Fixes: 98aaaec4a150 ("compat_ioctl: reimplement SG_IO handling")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
Not tested
---
 block/scsi_ioctl.c |  29 +----------
 drivers/scsi/sg.c  | 126 +++++++++++++++++++++------------------------
 include/scsi/sg.h  |  30 +++++++++++
 3 files changed, 91 insertions(+), 94 deletions(-)

-- 
2.20.0

Comments

Linus Torvalds Dec. 4, 2019, 6:32 p.m. | #1
On Wed, Dec 4, 2019 at 6:08 AM Arnd Bergmann <arnd@arndb.de> wrote:
>

> To address both of these, move the definition of compat_sg_io_hdr

> into a scsi/sg.h to make it visible to sg.c and rewrite the logic

> for reading req_pack_id as well as the size check to a simpler

> version that gets the expected results.


I think the patch is a good thing, except for this part:

> @@ -575,6 +561,14 @@ sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)

>         int err = 0, err2;

>         int len;

>

> +#ifdef CONFIG_COMPAT

> +       if (in_compat_syscall()) {

> +               if (count < sizeof(struct compat_sg_io_hdr)) {

> +                       err = -EINVAL;

> +                       goto err_out;

> +               }

> +       } else

> +#endif

>         if (count < SZ_SG_IO_HDR) {

>                 err = -EINVAL;

>                 goto err_out;


Yes, yes, I know we do things like that in some other places too, but
I really detest this kind of ifdeffery.

That

         } else
  #endif
         if (count < SZ_SG_IO_HDR) {

is just evil. Please don't add things like this where the #ifdef
section has subtle semantic continuations outside of it. If somebody
adds a statement in between there, it now acts completely wrong.

I think you can remove the #ifdef entirely. If CONFIG_COMPAT isn't
set, I think in_compat_syscall() just turns to 0, and the code gets
optimized away.

Hmm?

               Linus
Arnd Bergmann Dec. 4, 2019, 8:35 p.m. | #2
On Wed, Dec 4, 2019 at 7:33 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>

> On Wed, Dec 4, 2019 at 6:08 AM Arnd Bergmann <arnd@arndb.de> wrote:

> >

> > To address both of these, move the definition of compat_sg_io_hdr

> > into a scsi/sg.h to make it visible to sg.c and rewrite the logic

> > for reading req_pack_id as well as the size check to a simpler

> > version that gets the expected results.

>

> I think the patch is a good thing, except for this part:

>

> > @@ -575,6 +561,14 @@ sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)

> >         int err = 0, err2;

> >         int len;

> >

> > +#ifdef CONFIG_COMPAT

> > +       if (in_compat_syscall()) {

> > +               if (count < sizeof(struct compat_sg_io_hdr)) {

> > +                       err = -EINVAL;

> > +                       goto err_out;

> > +               }

> > +       } else

> > +#endif

> >         if (count < SZ_SG_IO_HDR) {

> >                 err = -EINVAL;

> >                 goto err_out;

>

> Yes, yes, I know we do things like that in some other places too, but

> I really detest this kind of ifdeffery.

>

> That

>

>          } else

>   #endif

>          if (count < SZ_SG_IO_HDR) {

>

> is just evil. Please don't add things like this where the #ifdef

> section has subtle semantic continuations outside of it. If somebody

> adds a statement in between there, it now acts completely wrong.

>

> I think you can remove the #ifdef entirely. If CONFIG_COMPAT isn't

> set, I think in_compat_syscall() just turns to 0, and the code gets

> optimized away.

>

> Hmm?


It almost works, but the part of the y2038 work that made all the
compat infrastructure visible on all architectures with or without
CONFIG_COMPAT never made it in after we decided to separate
the _time32 namespace from the compat_ namespace entirely.

It actually works on architectures that don't override asm/compat.h,
and on those that have CONFIG_COMPAT enabled, but for example
on arm64 with CONFIG_COMPAT=n I run into a build error because
asm-generic/compat.h is not included here, and getting that to
work reliably needed some rearranging of other files.

I could

a) dig out my old patches that did this right, so we can kill off
most of these #ifdefs in compat code throughout the kernel
(probably not this merge window),

b) change compat_sg_io_hdr to use plain types (u32, s32, ...), or

c) conditionally define another macro for SZ_COMPAT_SG_IO_HDR
like (pasted into gmail, won't apply)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index af152a7e71c7..039858014e18 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -198,6 +198,11 @@ static void sg_device_destroy(struct kref *kref);

 #define SZ_SG_HEADER sizeof(struct sg_header)
 #define SZ_SG_IO_HDR sizeof(sg_io_hdr_t)
+#ifdef CONFIG_COMPAT
+#define SZ_COMPAT_SG_IO_HDR SZ_SG_IO_HDR
+#else
+#define SZ_COMPAT_SG_IO_HDR sizeof(struct compat_sg_io_hdr)
+#endif
 #define SZ_SG_IOVEC sizeof(sg_iovec_t)
 #define SZ_SG_REQ_INFO sizeof(sg_req_info_t)

@@ -561,15 +566,12 @@ sg_new_read(Sg_fd * sfp, char __user *buf,
size_t count, Sg_request * srp)
        int err = 0, err2;
        int len;

-#ifdef CONFIG_COMPAT
        if (in_compat_syscall()) {
-               if (count < sizeof(struct compat_sg_io_hdr)) {
+               if (count < SZ_COMPAT_SG_IO_HDR) {
                        err = -EINVAL;
                        goto err_out;
                }
-       } else
-#endif
-       if (count < SZ_SG_IO_HDR) {
+       } else if (count < SZ_SG_IO_HDR) {
                err = -EINVAL;
                goto err_out;
        }

        Arnd

Patch

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 650bade5ea5a..b61dbf4d8443 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -20,6 +20,7 @@ 
 #include <scsi/scsi.h>
 #include <scsi/scsi_ioctl.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/sg.h>
 
 struct blk_cmd_filter {
 	unsigned long read_ok[BLK_SCSI_CMD_PER_LONG];
@@ -550,34 +551,6 @@  static inline int blk_send_start_stop(struct request_queue *q,
 	return __blk_send_generic(q, bd_disk, GPCMD_START_STOP_UNIT, data);
 }
 
-#ifdef CONFIG_COMPAT
-struct compat_sg_io_hdr {
-	compat_int_t interface_id;	/* [i] 'S' for SCSI generic (required) */
-	compat_int_t dxfer_direction;	/* [i] data transfer direction  */
-	unsigned char cmd_len;		/* [i] SCSI command length ( <= 16 bytes) */
-	unsigned char mx_sb_len;	/* [i] max length to write to sbp */
-	unsigned short iovec_count;	/* [i] 0 implies no scatter gather */
-	compat_uint_t dxfer_len;	/* [i] byte count of data transfer */
-	compat_uint_t dxferp;		/* [i], [*io] points to data transfer memory
-						or scatter gather list */
-	compat_uptr_t cmdp;		/* [i], [*i] points to command to perform */
-	compat_uptr_t sbp;		/* [i], [*o] points to sense_buffer memory */
-	compat_uint_t timeout;		/* [i] MAX_UINT->no timeout (unit: millisec) */
-	compat_uint_t flags;		/* [i] 0 -> default, see SG_FLAG... */
-	compat_int_t pack_id;		/* [i->o] unused internally (normally) */
-	compat_uptr_t usr_ptr;		/* [i->o] unused internally */
-	unsigned char status;		/* [o] scsi status */
-	unsigned char masked_status;	/* [o] shifted, masked scsi status */
-	unsigned char msg_status;	/* [o] messaging level data (optional) */
-	unsigned char sb_len_wr;	/* [o] byte count actually written to sbp */
-	unsigned short host_status;	/* [o] errors from host adapter */
-	unsigned short driver_status;	/* [o] errors from software driver */
-	compat_int_t resid;		/* [o] dxfer_len - actual_transferred */
-	compat_uint_t duration;		/* [o] time taken by cmd (unit: millisec) */
-	compat_uint_t info;		/* [o] auxiliary information */
-};
-#endif
-
 int put_sg_io_hdr(const struct sg_io_hdr *hdr, void __user *argp)
 {
 #ifdef CONFIG_COMPAT
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 160748ad9c0f..bc761059f1a8 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -405,6 +405,37 @@  sg_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static int get_sg_io_pack_id(int *pack_id, void __user *buf, size_t count)
+{
+	struct sg_header __user *old_hdr = buf;
+	int reply_len;
+
+	if (count < SZ_SG_HEADER)
+		goto unknown_id;
+
+	/* negative reply_len means v3 format, otherwise v1/v2 */
+	if (get_user(reply_len, &old_hdr->reply_len))
+		return -EFAULT;
+	if (reply_len >= 0)
+		return get_user(*pack_id, &old_hdr->pack_id);
+
+#ifdef CONFIG_COMPAT
+	if (in_compat_syscall() && count >= sizeof(struct compat_sg_io_hdr)) {
+		struct compat_sg_io_hdr __user *hp = buf;
+		return get_user(*pack_id, &hp->pack_id);
+	}
+#endif
+	if (count >= sizeof(struct sg_io_hdr)) {
+		struct sg_io_hdr __user *hp = buf;
+		return get_user(*pack_id, &hp->pack_id);
+	}
+
+unknown_id:
+	/* no valid header was passed, so ignore the pack_id */
+	*pack_id = -1;
+	return 0;
+}
+
 static ssize_t
 sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 {
@@ -413,8 +444,8 @@  sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 	Sg_request *srp;
 	int req_pack_id = -1;
 	sg_io_hdr_t *hp;
-	struct sg_header *old_hdr = NULL;
-	int retval = 0;
+	struct sg_header *old_hdr;
+	int retval;
 
 	/*
 	 * This could cause a response to be stranded. Close the associated
@@ -429,79 +460,34 @@  sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 	SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp,
 				      "sg_read: count=%d\n", (int) count));
 
-	if (sfp->force_packid && (count >= SZ_SG_HEADER)) {
-		old_hdr = memdup_user(buf, SZ_SG_HEADER);
-		if (IS_ERR(old_hdr))
-			return PTR_ERR(old_hdr);
-		if (old_hdr->reply_len < 0) {
-			if (count >= SZ_SG_IO_HDR) {
-				/*
-				 * This is stupid.
-				 *
-				 * We're copying the whole sg_io_hdr_t from user
-				 * space just to get the 'pack_id' field. But the
-				 * field is at different offsets for the compat
-				 * case, so we'll use "get_sg_io_hdr()" to copy
-				 * the whole thing and convert it.
-				 *
-				 * We could do something like just calculating the
-				 * offset based of 'in_compat_syscall()', but the
-				 * 'compat_sg_io_hdr' definition is in the wrong
-				 * place for that.
-				 */
-				sg_io_hdr_t *new_hdr;
-				new_hdr = kmalloc(SZ_SG_IO_HDR, GFP_KERNEL);
-				if (!new_hdr) {
-					retval = -ENOMEM;
-					goto free_old_hdr;
-				}
-				retval = get_sg_io_hdr(new_hdr, buf);
-				req_pack_id = new_hdr->pack_id;
-				kfree(new_hdr);
-				if (retval) {
-					retval = -EFAULT;
-					goto free_old_hdr;
-				}
-			}
-		} else
-			req_pack_id = old_hdr->pack_id;
-	}
+	if (sfp->force_packid)
+		retval = get_sg_io_pack_id(&req_pack_id, buf, count);
+	if (retval)
+		return retval;
+
 	srp = sg_get_rq_mark(sfp, req_pack_id);
 	if (!srp) {		/* now wait on packet to arrive */
-		if (atomic_read(&sdp->detaching)) {
-			retval = -ENODEV;
-			goto free_old_hdr;
-		}
-		if (filp->f_flags & O_NONBLOCK) {
-			retval = -EAGAIN;
-			goto free_old_hdr;
-		}
+		if (atomic_read(&sdp->detaching))
+			return -ENODEV;
+		if (filp->f_flags & O_NONBLOCK)
+			return -EAGAIN;
 		retval = wait_event_interruptible(sfp->read_wait,
 			(atomic_read(&sdp->detaching) ||
 			(srp = sg_get_rq_mark(sfp, req_pack_id))));
-		if (atomic_read(&sdp->detaching)) {
-			retval = -ENODEV;
-			goto free_old_hdr;
-		}
-		if (retval) {
+		if (atomic_read(&sdp->detaching))
+			return -ENODEV;
+		if (retval)
 			/* -ERESTARTSYS as signal hit process */
-			goto free_old_hdr;
-		}
-	}
-	if (srp->header.interface_id != '\0') {
-		retval = sg_new_read(sfp, buf, count, srp);
-		goto free_old_hdr;
+			return retval;
 	}
+	if (srp->header.interface_id != '\0')
+		return sg_new_read(sfp, buf, count, srp);
 
 	hp = &srp->header;
-	if (old_hdr == NULL) {
-		old_hdr = kmalloc(SZ_SG_HEADER, GFP_KERNEL);
-		if (! old_hdr) {
-			retval = -ENOMEM;
-			goto free_old_hdr;
-		}
-	}
-	memset(old_hdr, 0, SZ_SG_HEADER);
+	old_hdr = kzalloc(SZ_SG_HEADER, GFP_KERNEL);
+	if (!old_hdr)
+		return -ENOMEM;
+
 	old_hdr->reply_len = (int) hp->timeout;
 	old_hdr->pack_len = old_hdr->reply_len; /* old, strange behaviour */
 	old_hdr->pack_id = hp->pack_id;
@@ -575,6 +561,14 @@  sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
 	int err = 0, err2;
 	int len;
 
+#ifdef CONFIG_COMPAT
+	if (in_compat_syscall()) {
+		if (count < sizeof(struct compat_sg_io_hdr)) {
+			err = -EINVAL;
+			goto err_out;
+		}
+	} else
+#endif
 	if (count < SZ_SG_IO_HDR) {
 		err = -EINVAL;
 		goto err_out;
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index f91bcca604e4..c802739bde2f 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -68,6 +68,36 @@  typedef struct sg_io_hdr
     unsigned int info;          /* [o] auxiliary information */
 } sg_io_hdr_t;  /* 64 bytes long (on i386) */
 
+#if defined(__KERNEL__) && defined(CONFIG_COMPAT)
+#include <linux/compat.h>
+
+struct compat_sg_io_hdr {
+	compat_int_t interface_id;	/* [i] 'S' for SCSI generic (required) */
+	compat_int_t dxfer_direction;	/* [i] data transfer direction  */
+	unsigned char cmd_len;		/* [i] SCSI command length ( <= 16 bytes) */
+	unsigned char mx_sb_len;	/* [i] max length to write to sbp */
+	unsigned short iovec_count;	/* [i] 0 implies no scatter gather */
+	compat_uint_t dxfer_len;	/* [i] byte count of data transfer */
+	compat_uint_t dxferp;		/* [i], [*io] points to data transfer memory
+						or scatter gather list */
+	compat_uptr_t cmdp;		/* [i], [*i] points to command to perform */
+	compat_uptr_t sbp;		/* [i], [*o] points to sense_buffer memory */
+	compat_uint_t timeout;		/* [i] MAX_UINT->no timeout (unit: millisec) */
+	compat_uint_t flags;		/* [i] 0 -> default, see SG_FLAG... */
+	compat_int_t pack_id;		/* [i->o] unused internally (normally) */
+	compat_uptr_t usr_ptr;		/* [i->o] unused internally */
+	unsigned char status;		/* [o] scsi status */
+	unsigned char masked_status;	/* [o] shifted, masked scsi status */
+	unsigned char msg_status;	/* [o] messaging level data (optional) */
+	unsigned char sb_len_wr;	/* [o] byte count actually written to sbp */
+	unsigned short host_status;	/* [o] errors from host adapter */
+	unsigned short driver_status;	/* [o] errors from software driver */
+	compat_int_t resid;		/* [o] dxfer_len - actual_transferred */
+	compat_uint_t duration;		/* [o] time taken by cmd (unit: millisec) */
+	compat_uint_t info;		/* [o] auxiliary information */
+};
+#endif
+
 #define SG_INTERFACE_ID_ORIG 'S'
 
 /* Use negative values to flag difference from original sg_header structure */