[RFC] Read/Write Buffer support

[RFC] Read/Write Buffer support

[Matthew Wilcox]

This patch adds partial support for the SCSI commands READ_BUFFER and
WRITE_BUFFER.  Because ATA only supports a single 512-byte buffer per
drive, this isn't as useful as it could be, but it might prove interesting
when extended to handle the DOWNLOAD MICROCODE operations.

Comments?

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 8f0e8f2..f12403f 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1711,6 +1711,117 @@ void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
 	args->done(cmd);
 }
 
+/*
+ * This helper is used for three subcommands of READ_BUFFER.  Subcommand 0
+ * wants a header prefixing the data.  Subcommand 3 returns a descriptor
+ * and subcommand 0xb returns a descriptor for the echo buffer.  While these
+ * all have slightly different formats, in practise they all look sufficiently
+ * similar that this common function can be used.
+ */
+static void ata_scsi_fill_buffer_descriptor(struct scsi_cmnd *cmd, int desc)
+{
+	u8 *rbuf;
+	unsigned int buflen;
+	unsigned long flags;
+
+	local_irq_save(flags);
+
+	buflen = ata_scsi_rbuf_get(cmd, &rbuf);
+
+	memset(rbuf, 0, buflen);
+	rbuf[0] = desc ? 0xff : 0x0;
+	rbuf[1] = 0x0;
+	rbuf[2] = 0x2;
+	rbuf[3] = 0x0;
+
+	ata_scsi_rbuf_put(cmd, rbuf);
+
+	local_irq_restore(flags);
+}
+
+static int ata_scsi_read_buffer_descriptor(struct scsi_cmnd *cmd)
+{
+	ata_scsi_fill_buffer_descriptor(cmd, cmd->cmnd[1] == 0x3);
+
+	cmd->result = SAM_STAT_GOOD;
+	return 1;
+}
+
+static void ata_scsi_rwbuf_header(struct scsi_cmnd *cmd)
+{
+	struct scatterlist *sg = scsi_sglist(cmd);
+	ata_scsi_fill_buffer_descriptor(cmd, 0);
+
+	/* XXX: fewer than 4 bytes in the page?  Doomed. */
+	sg->offset += 4;
+	sg->length -= 4;
+}
+
+/**
+ * ata_scsi_rwbuf_xlat - Translate READ_BUFFER and WRITE_BUFFER
+ *
+ * SCSI has a very flexible READ_BUFFER and WRITE_BUFFER system.
+ * ATA's READ BUFFER only supports an offset of 0 and length 512.
+ * SCSI also uses WRITE_BUFFER for DOWNLOAD MICROCODE, which is not
+ * yet supported by this function.
+ */
+static unsigned int ata_scsi_rwbuf_xlat(struct ata_queued_cmd *qc)
+{
+	struct scsi_cmnd *scmd = qc->scsicmd;
+	const u8 *cdb = scmd->cmnd;
+	struct ata_taskfile *tf = &qc->tf;
+	int offset = 0;
+
+	if (cdb[0] == WRITE_BUFFER) {
+		tf->flags |= ATA_TFLAG_WRITE;
+		tf->command = ATA_CMD_WRITE_BUFFER;
+	} else {
+		tf->command = ATA_CMD_READ_BUFFER;
+	}
+
+	switch (cdb[1]) {
+	case 0x0:	/* Read/write data with header */
+		ata_scsi_rwbuf_header(scmd);
+		offset = 4;
+	case 0x2:	/* Read/write data */
+	case 0xa:	/* Read/write echo buffer */
+		break;
+	case 0x3:	/* Read descriptor */
+	case 0xb:	/* Read echo buffer descriptor */
+		if (cdb[0] == READ_BUFFER)
+			return ata_scsi_read_buffer_descriptor(scmd);
+	case 0x4:
+	case 0x5:
+	case 0x6:
+	case 0x7:
+		/* XXX: We can translate to DOWNLOAD_MICROCODE here */
+
+	default:
+		goto invalid_fld;
+	}
+
+	if ((cdb[2] != 0) || (cdb[3] != 0) || (cdb[4] != 0) || (cdb[5] != 0))
+		goto invalid_fld;
+	if (((cdb[6] << 16) | (cdb[7] << 8) | cdb[8]) != (512 + offset))
+		goto invalid_fld;
+
+	qc->flags |= ATA_QCFLAG_IO;
+	qc->nbytes = 512;
+	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf->protocol = ATA_PROT_PIO;
+	tf->nsect = 0;
+	tf->lbah = 0;
+	tf->lbam = 0;
+	tf->lbal = 0;
+
+	return 0;
+
+invalid_fld:
+	ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x24, 0x0);
+	/* "Invalid field in cbd" */
+	return 1;
+}
+
 /**
  *	ATA_SCSI_RBUF_SET - helper to set values in SCSI response buffer
  *	@idx: byte index into SCSI response buffer
@@ -2913,6 +3024,10 @@ static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
 	case WRITE_16:
 		return ata_scsi_rw_xlat;
 
+	case READ_BUFFER:
+	case WRITE_BUFFER:
+		return ata_scsi_rwbuf_xlat;
+
 	case SYNCHRONIZE_CACHE:
 		if (ata_try_flush_cache(dev))
 			return ata_scsi_flush_xlat;
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 1c622e2..fc77d6d 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -176,6 +176,8 @@ enum {
 	ATA_CMD_WRITE_MULTI_FUA_EXT = 0xCE,
 	ATA_CMD_SET_FEATURES	= 0xEF,
 	ATA_CMD_SET_MULTI	= 0xC6,
+	ATA_CMD_READ_BUFFER	= 0xE4,
+	ATA_CMD_WRITE_BUFFER	= 0xE8,
 	ATA_CMD_PACKET		= 0xA0,
 	ATA_CMD_VERIFY		= 0x40,
 	ATA_CMD_VERIFY_EXT	= 0x42,

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC] Read/Write Buffer support

[Jeff Garzik]

Matthew Wilcox wrote:
> This patch adds partial support for the SCSI commands READ_BUFFER and
> WRITE_BUFFER.  Because ATA only supports a single 512-byte buffer per
> drive, this isn't as useful as it could be, but it might prove interesting
> when extended to handle the DOWNLOAD MICROCODE operations.

General concept:  ACK

Minor nits below...


> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 8f0e8f2..f12403f 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1711,6 +1711,117 @@ void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
>  	args->done(cmd);
>  }
>  
> +/*
> + * This helper is used for three subcommands of READ_BUFFER.  Subcommand 0
> + * wants a header prefixing the data.  Subcommand 3 returns a descriptor
> + * and subcommand 0xb returns a descriptor for the echo buffer.  While these
> + * all have slightly different formats, in practise they all look sufficiently
> + * similar that this common function can be used.
> + */
> +static void ata_scsi_fill_buffer_descriptor(struct scsi_cmnd *cmd, int desc)
> +{
> +	u8 *rbuf;
> +	unsigned int buflen;
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +
> +	buflen = ata_scsi_rbuf_get(cmd, &rbuf);
> +
> +	memset(rbuf, 0, buflen);
> +	rbuf[0] = desc ? 0xff : 0x0;
> +	rbuf[1] = 0x0;
> +	rbuf[2] = 0x2;
> +	rbuf[3] = 0x0;
> +
> +	ata_scsi_rbuf_put(cmd, rbuf);
> +
> +	local_irq_restore(flags);
> +}

use ata_scsi_rbuf_fill() rather than recreating it manually


> +static int ata_scsi_read_buffer_descriptor(struct scsi_cmnd *cmd)
> +{
> +	ata_scsi_fill_buffer_descriptor(cmd, cmd->cmnd[1] == 0x3);
> +
> +	cmd->result = SAM_STAT_GOOD;
> +	return 1;
> +}
> +
> +static void ata_scsi_rwbuf_header(struct scsi_cmnd *cmd)
> +{
> +	struct scatterlist *sg = scsi_sglist(cmd);
> +	ata_scsi_fill_buffer_descriptor(cmd, 0);
> +
> +	/* XXX: fewer than 4 bytes in the page?  Doomed. */
> +	sg->offset += 4;
> +	sg->length -= 4;

I'm quite uncomfortable skipping any amount of validation.


> +/**
> + * ata_scsi_rwbuf_xlat - Translate READ_BUFFER and WRITE_BUFFER
> + *
> + * SCSI has a very flexible READ_BUFFER and WRITE_BUFFER system.
> + * ATA's READ BUFFER only supports an offset of 0 and length 512.
> + * SCSI also uses WRITE_BUFFER for DOWNLOAD MICROCODE, which is not
> + * yet supported by this function.
> + */
> +static unsigned int ata_scsi_rwbuf_xlat(struct ata_queued_cmd *qc)
> +{
> +	struct scsi_cmnd *scmd = qc->scsicmd;
> +	const u8 *cdb = scmd->cmnd;
> +	struct ata_taskfile *tf = &qc->tf;
> +	int offset = 0;
> +
> +	if (cdb[0] == WRITE_BUFFER) {
> +		tf->flags |= ATA_TFLAG_WRITE;
> +		tf->command = ATA_CMD_WRITE_BUFFER;
> +	} else {
> +		tf->command = ATA_CMD_READ_BUFFER;
> +	}
> +
> +	switch (cdb[1]) {
> +	case 0x0:	/* Read/write data with header */
> +		ata_scsi_rwbuf_header(scmd);
> +		offset = 4;
> +	case 0x2:	/* Read/write data */
> +	case 0xa:	/* Read/write echo buffer */
> +		break;
> +	case 0x3:	/* Read descriptor */
> +	case 0xb:	/* Read echo buffer descriptor */
> +		if (cdb[0] == READ_BUFFER)
> +			return ata_scsi_read_buffer_descriptor(scmd);
> +	case 0x4:
> +	case 0x5:
> +	case 0x6:
> +	case 0x7:
> +		/* XXX: We can translate to DOWNLOAD_MICROCODE here */

Style in libata is "FIXME" or "TODO" not "XXX".  Please help with 
consistency, it makes grepping easier.

Plus, I dunno about you, but I'm not a porn star :)

	Jeff

Re: [RFC] Read/Write Buffer support

[Matthew Wilcox]

On Sat, Mar 15, 2008 at 01:18:05PM -0400, Jeff Garzik wrote:
> >+	local_irq_save(flags);
> >+
> >+	buflen = ata_scsi_rbuf_get(cmd, &rbuf);
> >+
> >+	memset(rbuf, 0, buflen);
> >+	rbuf[0] = desc ? 0xff : 0x0;
> >+	rbuf[1] = 0x0;
> >+	rbuf[2] = 0x2;
> >+	rbuf[3] = 0x0;
> >+
> >+	ata_scsi_rbuf_put(cmd, rbuf);
> >+
> >+	local_irq_restore(flags);
> >+}
> 
> use ata_scsi_rbuf_fill() rather than recreating it manually

ata_scsi_rbuf_fill() calls args->done(cmd).  Now, I can split the common
part of ata_scsi_rbuf_fill into a different function if you'd like, but
see below.

> >+	/* XXX: fewer than 4 bytes in the page?  Doomed. */
> >+	sg->offset += 4;
> >+	sg->length -= 4;
> 
> I'm quite uncomfortable skipping any amount of validation.

Two things ... first, this is just a cheesy hack.  I'd like (but I don't
see) a generic way to say "I've filled in the first N bytes of this
scatterlist, please adjust and continue".  That's something that should
go in lib/scatterlist.c, IMO.

Second, and more seriously, libata already makes much worse assumptions
than this about scatterlists; it's just that the person who wrote them
didn't know it (or didn't flag it).  Look at the simulation of an
INQUIRY, for example.  It calls:

        ata_scsi_rbuf_fill(&args, ata_scsiop_inq_std);

which does:

        buflen = ata_scsi_rbuf_get(cmd, &rbuf);
	rc = actor(args, rbuf, buflen);

ata_scsi_rbuf_get() does:

        buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
	buflen = sg->length;

ata_scsiop_inq_std() (before it checks buflen) does:

        memcpy(rbuf, hdr, sizeof(hdr));

So if a user can persuade the kernel to do an SG transfer directly to
userspace and sets up the address one byte before the end of the page,
they can persuade libata to write 4 bytes into the page after KM_IRQ0.
Which is probably hard to exploit in a meaningful way, but could
certainly cause crashes and/or corruption.

> Style in libata is "FIXME" or "TODO" not "XXX".  Please help with 
> consistency, it makes grepping easier.

Fine by me.

> Plus, I dunno about you, but I'm not a porn star :)

Matthew "Spankalicious" Wilcox.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC] Read/Write Buffer support

[Jeff Garzik]

Matthew Wilcox wrote:
> On Sat, Mar 15, 2008 at 01:18:05PM -0400, Jeff Garzik wrote:
>>> +	local_irq_save(flags);
>>> +
>>> +	buflen = ata_scsi_rbuf_get(cmd, &rbuf);
>>> +
>>> +	memset(rbuf, 0, buflen);
>>> +	rbuf[0] = desc ? 0xff : 0x0;
>>> +	rbuf[1] = 0x0;
>>> +	rbuf[2] = 0x2;
>>> +	rbuf[3] = 0x0;
>>> +
>>> +	ata_scsi_rbuf_put(cmd, rbuf);
>>> +
>>> +	local_irq_restore(flags);
>>> +}
>> use ata_scsi_rbuf_fill() rather than recreating it manually
> 
> ata_scsi_rbuf_fill() calls args->done(cmd).  Now, I can split the common
> part of ata_scsi_rbuf_fill into a different function if you'd like, but
> see below.

No, you're right.  My bad:  rbuf_fill is for completely emulated 
commands, not translated commands.


> Second, and more seriously, libata already makes much worse assumptions
> than this about scatterlists; it's just that the person who wrote them
> didn't know it (or didn't flag it).  Look at the simulation of an
> INQUIRY, for example.  It calls:
[...]
> So if a user can persuade the kernel to do an SG transfer directly to
> userspace and sets up the address one byte before the end of the page,
> they can persuade libata to write 4 bytes into the page after KM_IRQ0.
> Which is probably hard to exploit in a meaningful way, but could
> certainly cause crashes and/or corruption.

You arrived at the proper conclusion -- it doesn't happen in practice 
for a variety of reasons, the main one being that it's largely in the 
realm of something the superuser must do intentionally, the way kernel 
and userland code is written today.

Thus, the code intentionally avoids the silly complexity for a case that 
never truly happens outside of synthetic superuser experiments.

Remember, this and all the other code originated in the days of "if 
(use_sg)", when scatterlists were not used often for !(read|write) commands.


>> Style in libata is "FIXME" or "TODO" not "XXX".  Please help with 
>> consistency, it makes grepping easier.
> 
> Fine by me.
> 
>> Plus, I dunno about you, but I'm not a porn star :)
> 
> Matthew "Spankalicious" Wilcox.

<grin>

	Jeff


P.S.  I encourage others to investigate making the libata SCSI simulator 
an optional kernel module.

Long term, a preferred solution would be to use SCSI layer for ATAPI 
devices, and a kernel block device for ATA devices.

	Jeff

Re: [RFC] Read/Write Buffer support

[Alan Cox]

> You arrived at the proper conclusion -- it doesn't happen in practice 
> for a variety of reasons, the main one being that it's largely in the 
> realm of something the superuser must do intentionally, the way kernel 
> and userland code is written today.

Actually thats a serious privilege escalation as it gets you from raw
disk access to cap_sys_rawio so its a security flaw and should be treated
as such even though it is only a minor one in most operating contexts.

Alan

Re: [RFC] Read/Write Buffer support

[Matthew Wilcox]

On Sat, Mar 15, 2008 at 06:48:03PM -0400, Jeff Garzik wrote:
> You arrived at the proper conclusion -- it doesn't happen in practice 
> for a variety of reasons, the main one being that it's largely in the 
> realm of something the superuser must do intentionally, the way kernel 
> and userland code is written today.

I think it could easily happen accidentally.  In any case, it's worth
fxing.

> Thus, the code intentionally avoids the silly complexity for a case that 
> never truly happens outside of synthetic superuser experiments.
> 
> Remember, this and all the other code originated in the days of "if 
> (use_sg)", when scatterlists were not used often for !(read|write) commands.

I have no desire to increase the complexity of libata, but I think it's
possible to fix this in a couple of different ways without adding extra
complexity.

One possible way would be for simulate to allocate a page and pass that
into the actor functions to fill in.  Then simulate could copy that page
into the sg lists.  This doesn't fix my problem where I need to
part-simulate, part-translate, but it's a possible solution for all the
various inquiries.

Another would be for the actor functions to set up their own buffers and
call something similar to the copy_buffer() function I had as part of
the scsi_ram driver I wrote.  I happened to write it for the use of
INQUIRY too ;-)

+static void copy_buffer(struct scsi_cmnd *cmnd, char *buf, int len)
+{
+       char *p;
+       struct scatterlist *sg;
+       int i;
+
+       scsi_for_each_sg(cmnd, sg, scsi_sg_count(cmnd), i) {
+               int tocopy = sg->length;
+               if (tocopy > len)
+                       tocopy = len;
+
+               p = kmap_atomic(sg_page(sg), KM_USER0);
+               memcpy(p + sg->offset, buf, tocopy);
+               kunmap_atomic(p, KM_USER0);
+
+               len -= tocopy;
+               if (!len)
+                       break;
+               buf += tocopy;
+       }
+
+       scsi_set_resid(cmnd, len);
+}

> P.S.  I encourage others to investigate making the libata SCSI simulator 
> an optional kernel module.
> 
> Long term, a preferred solution would be to use SCSI layer for ATAPI 
> devices, and a kernel block device for ATA devices.

I think that's the right method to take, though we probably still want
to emulate the scsi ioctls.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC] Read/Write Buffer support

[Jeff Garzik]

Matthew Wilcox wrote:
> On Sat, Mar 15, 2008 at 06:48:03PM -0400, Jeff Garzik wrote:
>> P.S.  I encourage others to investigate making the libata SCSI simulator 
>> an optional kernel module.
>>
>> Long term, a preferred solution would be to use SCSI layer for ATAPI 
>> devices, and a kernel block device for ATA devices.
> 
> I think that's the right method to take, though we probably still want
> to emulate the scsi ioctls.

We'll want the ATA<->SCSI simulator in its entirety as an optional 
module for compatibility.  People have built applications assuming they 
must use SCSI (more specifically, SAT) to probe and manipulate ATA devices.

	Jeff