| Message ID | 577e843e.0a86620a.59167.ffffb899@mx.google.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2016-07-08 at 00:32 +0800, tom.ty89@gmail.com wrote: > From: Tom Yan <tom.ty89@gmail.com> > > With this patch, users can make use of the SANITIZE DEVICE feature > set through utility like sg_sanitize. > > Support for BLOCK ERASE, CRYPTOGRAPHIC ERASE and EXIT FAILURE MODE > has been implemented. Support for OVERWRITE that involves a > parameter list has been left out for now. > > Further support for command with IMMED bit set to zero, REQUEST > SENSE translation for user-space status polling, and support > checking in IDENTIFY DEVICE data log (return proper sense data > when designated method is not supported) should be implemented > in the future as well. > > `sg_sanitize -e -B|-C|-F /dev/sdX` should work fine with this. Why on earth would you want to do this? If your intent is to sanitise the disk using a cryptographic erase you presumably have a real security need for doing it and, knowing what goes into most SAT layers, I'd not really trust any SAT for this operation, so for an underlying SATA device I'd use ATA_16 to send a real ACS-2 SANITIZE command. Just as a general note about our SAT layer: Adding little used features is an invitation to bloat it with buggy implementations which makes it harder to understand and bug prone for odd and unlikely use cases, which then take ages to diagnose and track down. The only things which should be in the SAT is what the Linux SCSI subsystem would actually use. For everything else, if the user cares enough, they'll send down an encapsulated ATA command. James -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
To be honest, that sounds like FUD to me. The exact reason why this can "safely" be introduced to the SATL is that, it is a "one-shot" command that would only be triggered by the user through a user space utility. It's nothing like TRIM that would be triggered by the filesystem layer or so "from time to time", which might cost users "unexpected" data loss. It is also nothing like SECURE ERASE (which vendors had to abuse to provide equivalence of BLOCK ERASE and CRYPTOGRAPHIC ERASE for SSDs and drives that does hardware encryption) that requires the drive to be locked before you can issue an erase command. Certainly you can expect users to pack an ATA PASS-THROUGH command themselves and issued it with sg_raw or so, or hdparm to be patched to support the full feature set (including the "optional" FREEZE/ANTIFREEZE sub-feature), but I don't see how these would be reasons that the translation should not be introduced to the kernel, so that we can make good use of its existing SATL infrastructure and sg_sanitize. Whether it is "secure" enough to use the implementation would be the user's call. To be frank, saying that implementing it in the "SAT-way" would make it untrustable doesn't make any sense to me, especially when the way the feature set works is considered. On 8 July 2016 at 00:47, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Fri, 2016-07-08 at 00:32 +0800, tom.ty89@gmail.com wrote: >> From: Tom Yan <tom.ty89@gmail.com> >> >> With this patch, users can make use of the SANITIZE DEVICE feature >> set through utility like sg_sanitize. >> >> Support for BLOCK ERASE, CRYPTOGRAPHIC ERASE and EXIT FAILURE MODE >> has been implemented. Support for OVERWRITE that involves a >> parameter list has been left out for now. >> >> Further support for command with IMMED bit set to zero, REQUEST >> SENSE translation for user-space status polling, and support >> checking in IDENTIFY DEVICE data log (return proper sense data >> when designated method is not supported) should be implemented >> in the future as well. >> >> `sg_sanitize -e -B|-C|-F /dev/sdX` should work fine with this. > > Why on earth would you want to do this? If your intent is to sanitise > the disk using a cryptographic erase you presumably have a real > security need for doing it and, knowing what goes into most SAT layers, > I'd not really trust any SAT for this operation, so for an underlying > SATA device I'd use ATA_16 to send a real ACS-2 SANITIZE command. > > Just as a general note about our SAT layer: Adding little used features > is an invitation to bloat it with buggy implementations which makes it > harder to understand and bug prone for odd and unlikely use cases, > which then take ages to diagnose and track down. The only things which > should be in the SAT is what the Linux SCSI subsystem would actually > use. For everything else, if the user cares enough, they'll send down > an encapsulated ATA command. > > James > > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2016-07-09 at 00:20 +0800, Tom Yan wrote: > To be honest, that sounds like FUD to me. If argument is to be in acronyms, it's KISS. > The exact reason why this can "safely" be introduced to the SATL is > that, it is a "one-shot" command that would only be triggered by the > user through a user space utility. It's nothing like TRIM that would > be triggered by the filesystem layer or so "from time to time", which > might cost users "unexpected" data loss. It is also nothing like > SECURE ERASE (which vendors had to abuse to provide equivalence of > BLOCK ERASE and CRYPTOGRAPHIC ERASE for SSDs and drives that does > hardware encryption) that requires the drive to be locked before you > can issue an erase command. OK, since you ignore the argument about maintenance: safety for us means that it doesn't bitrot as an almost never used addition. The reason our SATL should only support the commands Linux uses is precisely because if it's used often we get immediate notice of when we break it. This maintenance burden means that adding stuff isn't free so we should have some utility bar before we do it. Just "because we can" doesn't seem to rise to that. > Certainly you can expect users to pack an ATA PASS-THROUGH command > themselves and issued it with sg_raw or so, or hdparm to be patched > to support the full feature set (including the "optional" > FREEZE/ANTIFREEZE sub-feature), but I don't see how these would be > reasons that the translation should not be introduced to the kernel, > so that we can make good use of its existing SATL infrastructure and > sg_sanitize. Or we could simply patch sg_sanitze to issue the ATA_16 pass through when it sees a sata device ... > Whether it is "secure" enough to use the implementation would be the > user's call. To be frank, saying that implementing it in the "SAT > -way" > would make it untrustable doesn't make any sense to me, especially > when the way the feature set works is considered. To be honest, I bet real security people won't even trust the drive firmware. Their answer will still be dd random patterns to prevent easy retrieval then crush the drive to prevent forensic retrieval. James > On 8 July 2016 at 00:47, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > On Fri, 2016-07-08 at 00:32 +0800, tom.ty89@gmail.com wrote: > > > From: Tom Yan <tom.ty89@gmail.com> > > > > > > With this patch, users can make use of the SANITIZE DEVICE > > > feature > > > set through utility like sg_sanitize. > > > > > > Support for BLOCK ERASE, CRYPTOGRAPHIC ERASE and EXIT FAILURE > > > MODE > > > has been implemented. Support for OVERWRITE that involves a > > > parameter list has been left out for now. > > > > > > Further support for command with IMMED bit set to zero, REQUEST > > > SENSE translation for user-space status polling, and support > > > checking in IDENTIFY DEVICE data log (return proper sense data > > > when designated method is not supported) should be implemented > > > in the future as well. > > > > > > `sg_sanitize -e -B|-C|-F /dev/sdX` should work fine with this. > > > > Why on earth would you want to do this? If your intent is to > > sanitise > > the disk using a cryptographic erase you presumably have a real > > security need for doing it and, knowing what goes into most SAT > > layers, > > I'd not really trust any SAT for this operation, so for an > > underlying > > SATA device I'd use ATA_16 to send a real ACS-2 SANITIZE command. > > > > Just as a general note about our SAT layer: Adding little used > > features > > is an invitation to bloat it with buggy implementations which makes > > it > > harder to understand and bug prone for odd and unlikely use cases, > > which then take ages to diagnose and track down. The only things > > which > > should be in the SAT is what the Linux SCSI subsystem would > > actually > > use. For everything else, if the user cares enough, they'll send > > down > > an encapsulated ATA command. > > > > James > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" > in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8 July 2016 at 17:29, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > OK, since you ignore the argument about maintenance: safety for us > means that it doesn't bitrot as an almost never used addition. The > reason our SATL should only support the commands Linux uses is > precisely because if it's used often we get immediate notice of when we > break it. This maintenance burden means that adding stuff isn't free > so we should have some utility bar before we do it. Just "because we > can" doesn't seem to rise to that. > Well yeah it might rot just like other parts of the current SATL, but at least its rot would be self-contained. I don't really find it "an almost never used addition" btw. It's just a handy feature set that no one has implemented any interface for users to make use of it. > > Or we could simply patch sg_sanitze to issue the ATA_16 pass through > when it sees a sata device ... > Ugh that sounds ugly to me. Anyway that's off-topic. > > To be honest, I bet real security people won't even trust the drive > firmware. Their answer will still be dd random patterns to prevent > easy retrieval then crush the drive to prevent forensic retrieval. > You know what, I don't think the feature set is totally about "security" anyway. It's just a handy way quickly (that's partly why I didn't bother to implement translation for OVERWRITE) erase all the data. Also, whether one considers it secure enough or not would be purely his opinion under the usage context of his own, as always. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2016-07-08 at 19:38 +0000, Tom Yan wrote: > On 8 July 2016 at 17:29, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > Or we could simply patch sg_sanitze to issue the ATA_16 pass > > through when it sees a sata device ... > > > > Ugh that sounds ugly to me. Anyway that's off-topic. Not really. The point is that you've proposed something as an addition to the kernel that can also be done in userspace. Checking if it can work easily there is like a barrier to entry. If it works, then fine, we're done. If it throws up problems then we reconsider the kernel route. James -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I don't suppose there would be any problem doing it in userspace / with ATA PASS-THROUGH anyway. I just couldn't agree that it would be the reason not to implement the translation (which covers the core part of the feature set) in the kernel. But certainly I wouldn't keep aruging on this. I don't think we would really want to patch sg_sanitize the way you proposed, otherwise we would have made the SCSI disk driver doing ATA IDENTIFY DEVICE and issue TRIM commands directly, instead of relying on a SATL. ATA PASS-THROUGH should never be "triggered" like that anyway. Well, it is alright to have an "sg_sat_sanitize" (which makes use of ATA PASS-THROUGH like other sg_sat_*) though. I am just not sure if sg3_utils should go on having more sg_sat_*... On 9 July 2016 at 08:49, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Fri, 2016-07-08 at 19:38 +0000, Tom Yan wrote: >> On 8 July 2016 at 17:29, James Bottomley >> <James.Bottomley@hansenpartnership.com> wrote: >> > Or we could simply patch sg_sanitze to issue the ATA_16 pass >> > through when it sees a sata device ... >> > >> >> Ugh that sounds ugly to me. Anyway that's off-topic. > > Not really. The point is that you've proposed something as an addition > to the kernel that can also be done in userspace. Checking if it can > work easily there is like a barrier to entry. If it works, then fine, > we're done. If it throws up problems then we reconsider the kernel > route. > > James > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16-07-11 02:35 AM, Tom Yan wrote: > I don't suppose there would be any problem doing it in userspace / > with ATA PASS-THROUGH anyway. .. >>> On 8 July 2016 at 17:29, James Bottomley >>> <James.Bottomley@hansenpartnership.com> wrote: .. >> Not really. The point is that you've proposed something as an addition >> to the kernel that can also be done in userspace. Checking if it can >> work easily there is like a barrier to entry. If it works, then fine, >> we're done. If it throws up problems then we reconsider the kernel >> route. hdparm has full support for the SANITIZE commands in userspace. -ml -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index bfec66f..a64991b 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -3346,6 +3346,63 @@ invalid_opcode: return 1; } +static unsigned int ata_scsi_sanitize_xlat(struct ata_queued_cmd *qc) { + struct ata_taskfile *tf = &qc->tf; + struct scsi_cmnd *scmd = qc->scsicmd; + struct ata_device *dev = qc->dev; + const u8 *cdb = scmd->cmnd; + u16 fp; + u8 bp = 0xff; + + /* for now we only support SANITIZE with IMMED bit set */ + if (unlikely(!(cdb[1] & 0x80))) { + fp = 1; + bp = 7; + goto invalid_fld; + } + + tf->protocol = ATA_PROT_NODATA; + tf->command = ATA_CMD_SANITIZE_DEVICE; + tf->hob_nsect |= (cdb[1] & 0x40) << 1; + tf->nsect |= (cdb[1] & 0x20) >> 1; + tf->flags |= ATA_TFLAG_LBA48 | ATA_TFLAG_ISADDR; + + switch (cdb[1] & 0x1f) { + /* TODO: add support for OVERWRITE */ + case 0x2: /* BLOCK ERASE */ + tf->hob_feature = 0x0; + tf->feature = 0x12; + tf->hob_lbal = 0x42; + tf->lbah = 0x6b; + tf->lbam = 0x45; + tf->lbal = 0x72; + break; + case 0x3: /* CRYPTOGRAPHIC ERASE */ + tf->hob_feature = 0x0; + tf->feature = 0x11; + tf->hob_lbal = 0x43; + tf->lbah = 0x72; + tf->lbam = 0x79; + tf->lbal = 0x70; + break; + case 0x1f: /* EXIT FAILURE MODE */ + tf->hob_feature = 0x0; + tf->feature = 0x0; + tf->nsect |= 0x1; + break; + default: + fp = 1; + bp = 4; + goto invalid_fld; + } + + return 0; + +invalid_fld: + ata_scsi_set_invalid_field(dev, scmd, fp, bp); + return 1; +} + /** * ata_scsi_report_zones_complete - convert ATA output * @qc: command structure returning the data @@ -3869,6 +3926,9 @@ static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd) case WRITE_SAME_16: return ata_scsi_write_same_xlat; + case SANITIZE: + return ata_scsi_sanitize_xlat; + case SYNCHRONIZE_CACHE: if (ata_try_flush_cache(dev)) return ata_scsi_flush_xlat; diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h index d1defd1..20d6085 100644 --- a/include/scsi/scsi_proto.h +++ b/include/scsi/scsi_proto.h @@ -72,6 +72,7 @@ #define UNMAP 0x42 #define READ_TOC 0x43 #define READ_HEADER 0x44 +#define SANITIZE 0x48 #define GET_EVENT_STATUS_NOTIFICATION 0x4a #define LOG_SELECT 0x4c #define LOG_SENSE 0x4d