Re: [PATCH 5.5 284/367] compat: scsi: sg: fix v3 compat read/write interface

From: Naresh Kamboju
Date: Mon Feb 10 2020 - 10:41:48 EST

Next message: John Garry: "Re: [PATCH RFC 4/7] perf pmu: Rename uncore symbols to include system PMUs"
Previous message: Daniel W. S. Almeida: "[PATCH v5] media: dvb_dummy_tuner: implement driver skeleton"
In reply to: Greg Kroah-Hartman: "[PATCH 5.5 284/367] compat: scsi: sg: fix v3 compat read/write interface"
Next in thread: Arnd Bergmann: "Re: [PATCH 5.5 284/367] compat: scsi: sg: fix v3 compat read/write interface"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

The arm64 architecture 64k page size enabled build failed on stable rc 5.5
CONFIG_ARM64_64K_PAGES=y
CROSS_COMPILE=aarch64-linux-gnu-
Toolchain gcc-9

In file included from ../block/scsi_ioctl.c:23:
../include/scsi/sg.h:75:2: error: unknown type name âcompat_int_tâ
compat_int_t interface_id; /* [i] 'S' for SCSI generic (required) */
^~~~~~~~~~~~
../include/scsi/sg.h:76:2: error: unknown type name âcompat_int_tâ
compat_int_t dxfer_direction; /* [i] data transfer direction */
^~~~~~~~~~~~

...
../include/scsi/sg.h:97:2: error: unknown type name âcompat_uint_tâ
compat_uint_t info; /* [o] auxiliary information */
^~~~~~~~~~~~~
make[2]: *** [../scripts/Makefile.build:266: block/bsg.o] Error

Ref:
https://gitlab.com/Linaro/lkft/kernel-runs/-/jobs/431659186

On Mon, 10 Feb 2020 at 18:18, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> commit 78ed001d9e7106171e0ee761cd854137dd731302 upstream.
>
> 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")
> Reviewed-by: Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
> ---
> block/scsi_ioctl.c | 29 ------------
> drivers/scsi/sg.c | 126 ++++++++++++++++++++++++-----------------------------
> include/scsi/sg.h | 30 ++++++++++++
> 3 files changed, 90 insertions(+), 95 deletions(-)
>
> --- 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(st
> 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
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -405,6 +405,38 @@ sg_release(struct inode *inode, struct f
> 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) {
> + /* 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);
> +
> + 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);
> + }
> +
> + if (count >= sizeof(struct sg_io_hdr)) {
> + struct sg_io_hdr __user *hp = buf;
> +
> + return get_user(*pack_id, &hp->pack_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 +445,8 @@ sg_read(struct file *filp, char __user *
> 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 +461,34 @@ sg_read(struct file *filp, char __user *
> 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,7 +562,12 @@ sg_new_read(Sg_fd * sfp, char __user *bu
> int err = 0, err2;
> int len;
>
> - if (count < SZ_SG_IO_HDR) {
> + if (in_compat_syscall()) {
> + if (count < sizeof(struct compat_sg_io_hdr)) {
> + err = -EINVAL;
> + goto err_out;
> + }
> + } else if (count < SZ_SG_IO_HDR) {
> err = -EINVAL;
> goto err_out;
> }
> --- 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__)
> +#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 */
>
>

--
Linaro LKFT
https://lkft.linaro.org

Next message: John Garry: "Re: [PATCH RFC 4/7] perf pmu: Rename uncore symbols to include system PMUs"
Previous message: Daniel W. S. Almeida: "[PATCH v5] media: dvb_dummy_tuner: implement driver skeleton"
In reply to: Greg Kroah-Hartman: "[PATCH 5.5 284/367] compat: scsi: sg: fix v3 compat read/write interface"
Next in thread: Arnd Bergmann: "Re: [PATCH 5.5 284/367] compat: scsi: sg: fix v3 compat read/write interface"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]