[PATCH] st: reject MTIOCTOP mt_count values out of range for the SPACE(6) scsi command

[PATCH] st: reject MTIOCTOP mt_count values out of range for the SPACE(6) scsi command

[Patrick Strateman]

Values greater than 0x7FFFFF do not fit in the 24 bit big endian two's
complement integer for the underlying scsi SPACE(6) command.

Signed-off-by: Patrick Strateman <patrick.strateman@gmail.com>
---
 drivers/scsi/st.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 43f7624508a9..190fa678d149 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -2719,6 +2719,22 @@ static int st_int_ioctl(struct scsi_tape *STp,
unsigned int cmd_in, unsigned lon
     blkno = STps->drv_block;
     at_sm = STps->at_sm;

+    switch (cmd_in) {
+    case MTFSFM:
+    case MTFSF:
+    case MTBSFM:
+    case MTBSF:
+    case MTFSR:
+    case MTBSR:
+    case MTFSS:
+    case MTBSS:
+        // count field for SPACE(6) is a 24 bit big endian two's
complement integer
+        if (arg > 0x7FFFFF) {
+            st_printk(ST_DEB_MSG, STp, "Cannot space more than
0x7FFFFF units.\n");
+            return (-EINVAL);
+        }
+    }
+
     memset(cmd, 0, MAX_COMMAND_SIZE);
     switch (cmd_in) {
     case MTFSFM:

Re: [PATCH] st: reject MTIOCTOP mt_count values out of range for the SPACE(6) scsi command

[Martin K. Petersen]

Patrick,

> Values greater than 0x7FFFFF do not fit in the 24 bit big endian two's
> complement integer for the underlying scsi SPACE(6) command.

I was hoping Kai would chime in.

However, since SPACE(6) has been deprecated for a while, I am not so
keen on blindly enforcing this in the ioctl interface. I would rather
see SPACE(16) support added and then have the supplied count value be
validated based on whether the 6 or 16 byte command is being issued to
the device.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] st: reject MTIOCTOP mt_count values out of range for the SPACE(6) scsi command

[Patrick Strateman]

I'm happy to implement SPACE(16) instead.

I believe the opcode for SPACE(16) is 0x91, but in
include/scsi/scsi_proto.h that value is listed as
SYNCHRONIZE_CACHE_16.

I couldn't figure out which was wrong, so I proposed this to at least
prevent the interface from doing the wrong thing.

On Tue, Jan 26, 2021 at 11:26 PM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Patrick,
>
> > Values greater than 0x7FFFFF do not fit in the 24 bit big endian two's
> > complement integer for the underlying scsi SPACE(6) command.
>
> I was hoping Kai would chime in.
>
> However, since SPACE(6) has been deprecated for a while, I am not so
> keen on blindly enforcing this in the ioctl interface. I would rather
> see SPACE(16) support added and then have the supplied count value be
> validated based on whether the 6 or 16 byte command is being issued to
> the device.
>
> --
> Martin K. Petersen      Oracle Linux Engineering

Re: [PATCH] st: reject MTIOCTOP mt_count values out of range for the SPACE(6) scsi command

[Martin K. Petersen]

Patrick,

> I believe the opcode for SPACE(16) is 0x91,

That is correct.

> but in include/scsi/scsi_proto.h that value is listed as
> SYNCHRONIZE_CACHE_16.

Yeah, I'm afraid things have a tendency to be SPC/SBC centric. Feel free
to add SPACE(16) and make a comment about the SSC vs. SBC distinction.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] st: reject MTIOCTOP mt_count values out of range for the SPACE(6) scsi command

["Kai Mäkisara (Kolumbus)"]

I am sorry that this is response has taken too long…

Martin suggested adding support for SPACE(16) and you said that you are willing to implement
that. I think this is the correct direction. I think that there are still in use old drives that don’t
support SPACE(16) and it is good if these are not forgotten.

In case you want the interim patch applied to prevent incorrect results with SPACE(6), I support that.

Thanks,
Kai


> On 13. Jan 2021, at 4.24, Patrick Strateman <patrick.strateman@gmail.com> wrote:
> 
> Values greater than 0x7FFFFF do not fit in the 24 bit big endian two's
> complement integer for the underlying scsi SPACE(6) command.
> 
> Signed-off-by: Patrick Strateman <patrick.strateman@gmail.com>

Acked-by: Kai Mäkisara <kai.makisara@kolumbus.fi>

> ---
> drivers/scsi/st.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 43f7624508a9..190fa678d149 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -2719,6 +2719,22 @@ static int st_int_ioctl(struct scsi_tape *STp,
> unsigned int cmd_in, unsigned lon
>     blkno = STps->drv_block;
>     at_sm = STps->at_sm;
> 
> +    switch (cmd_in) {
> +    case MTFSFM:
> +    case MTFSF:
> +    case MTBSFM:
> +    case MTBSF:
> +    case MTFSR:
> +    case MTBSR:
> +    case MTFSS:
> +    case MTBSS:
> +        // count field for SPACE(6) is a 24 bit big endian two's
> complement integer
> +        if (arg > 0x7FFFFF) {
> +            st_printk(ST_DEB_MSG, STp, "Cannot space more than
> 0x7FFFFF units.\n");
> +            return (-EINVAL);
> +        }
> +    }
> +
>     memset(cmd, 0, MAX_COMMAND_SIZE);
>     switch (cmd_in) {
>     case MTFSFM: