Repository navigation
Conversation
eae0f6d to
52d14eb
Compare
skr4n
left a comment
There was a problem hiding this comment.
It's best to defer this for a major release.
https://xkcd.com/1172/
| typedef enum { | ||
| CDIO_MMC_LEVEL_WEIRD, | ||
| CDIO_MMC_LEVEL_1 = 0x2, /* SCSI-2 */ | ||
| CDIO_MMC_LEVEL_1a = 0x3, /* SPC-1 / ANSI X3.301-1997 */ | ||
| CDIO_MMC_LEVEL_2 = 0x4, /* SPC-2 / ANSI INCITS 351-200 */ | ||
| CDIO_MMC_LEVEL_3 = 0x5, /* SPC-3 / ANSI INCITS 408-2005 */ | ||
| CDIO_MMC_LEVEL_45 = 0x6, /* SPC-4/5 */ | ||
| CDIO_MMC_LEVEL_5 = 0x7, /* SPC-5 / MMC-5/6 */ | ||
| CDIO_MMC_LEVEL_NONE | ||
| } cdio_mmc_level_t; | ||
|
|
There was a problem hiding this comment.
This would be a breaking change:
- The enum's moved to a new header
- The existing variants now have specific discriminants, which are different from the compiler assigned ones before.
There was a problem hiding this comment.
Okay, noted.
So then what I'll do is rename this enum to cdio_mmc_inquiry_version , and deprecate cdio_mmc_level_t.
Fixing this will add a new feature and deprecate an API feature. So I believe it will be in bounds for a minor release update.
| CDIO_MMC_GPCMD_INQUIRY, | ||
| 0x00, /* EVPD = 0, standard INQUIRY */ | ||
| 0x00, /* Page code (unused when EVPD=0) */ | ||
| 0x00, (uint8_t)buf_len, /* Allocation length */ |
There was a problem hiding this comment.
This would cause an out of bounds write:
- The allocation length is a two byte field, at indexes 3 and 4.
- Here,
buf_lenis assigned at index 3. - The big endian interpretation of that would be a much larger buffer than what's allocated in
buf.
There was a problem hiding this comment.
Including a test should catch things like this.
There was a problem hiding this comment.
This would cause an out of bounds write:
- The allocation length is a two byte field, at indexes 3 and 4.
- Here,
buf_lenis assigned at index 3.- The big endian interpretation of that would be a much larger buffer than what's allocated in
buf.
Good catch. Mysteriously, I've tested it, and it worked even though the length was set very high.
There was a problem hiding this comment.
The response data in your case must have fit the allocated buffer (252 bytes).
From my experience, an out-of-bounds write like this used to freeze the program and render the drive unresponsive until it was re-plugged.
| scsi_version = buf[2] & 0x07; | ||
|
|
||
| switch (scsi_version) { | ||
| case CDIO_MMC_LEVEL_1: | ||
| case CDIO_MMC_LEVEL_1a: | ||
| case CDIO_MMC_LEVEL_2: | ||
| case CDIO_MMC_LEVEL_3: | ||
| case CDIO_MMC_LEVEL_45: | ||
| case CDIO_MMC_LEVEL_5: | ||
| return (cdio_mmc_level_t)scsi_version; | ||
| default: |
There was a problem hiding this comment.
This would be incorrect:
- Here, the MMC version is being deduced from the SCSI version reported by the device.
- However, there can be devices that implement SCSI (SPC), but not SCSI (MMC).
- Instead, the
Version descriptorsfield (at indexes58..73) must be used, which report the exact MMC version, down to a revision level.
There was a problem hiding this comment.
However, this method may not be fully reliable:
- I've found that my drive (A Verbatim brand CD/DVD writer) which does respond to MMC commands, reports to not comply with any of the MMC levels..
- This is probably because the manufacturer does implement MMC, but perhaps only a subset of it (or a customized form of it).
For cases like my drive, the MMC_LEVEL_WEIRD variant would help, but it'd have to determine this using a different approach, which I'm not sure of yet.
| printf("MMC 1\n"); | ||
| case CDIO_MMC_LEVEL_1a: | ||
| printf("MMC-1 (CD)\n"); | ||
| break; | ||
| case CDIO_MMC_LEVEL_2: | ||
| printf("MMC 2\n"); | ||
| printf("MMC-2 (DVD)\n"); | ||
| break; | ||
| case CDIO_MMC_LEVEL_3: | ||
| printf("MMC 3\n"); | ||
| printf("SPC-3 / MMC-3 to MMC-5\n"); | ||
| break; | ||
| case CDIO_MMC_LEVEL_45: | ||
| printf("SPC-4 / MMC-5\n"); | ||
| break; | ||
| case CDIO_MMC_LEVEL_5: | ||
| printf("SPC-5 / MMC-5/6 (CD/DVD/BD)"); |
There was a problem hiding this comment.
This would be a breaking change too.
Also clang-format mmc.c
…rive_mmc_cap Also, set length in INQUERY command correctly. Include mmc_get_drive_mmc_cap_from_inquiry_version() inside mmc_read.c test
52d14eb to
9ad9c64
Compare
This is a low-level MMC command, not a high-level one. Also, use the existing convention for setting up a MMC command.
|
Having tried several things, I am thinking now that this is best put off until after the next release. Changing this to a draft now, for sometime later to come back to. |
Revise mmc-get-drive-cap to use the MMC/SCSCI
INQUIRYcommand.Also clang-format
mmc.c.Fixes #70