Page MenuHomeFreeBSD

camcontrol: possibly wrong sector count in ata_do_identify()
ClosedPublic

Authored by danfe on Mar 1 2019, 1:53 AM.

Details

Summary

I've noticed that in the ata_do_identify() function that was added in rS249115 issues ATA_ATA_IDENTIFY (0xec) command which reads a sector (512 bytes) of drive identification data (struct ata_params), but passes (u_int8_t)sizeof(struct ata_params) as sector_count which evaluates to zero.

This looks at least bogus, if not wrong. All the similar code I've seen, including e.g. that of popular Linux hdparm utility, supply 1 (one) as a sector count in this case. Even if zero is correct, it should not be obfuscated as a cast of 512 to u_int8_t which is too large and silently gets truncated.

I've tried to contact @smh several times about the matter, but he never replied, so apparently he's no longer active with the project, thus I'm putting it up for review here.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

danfe created this revision.Mar 1 2019, 1:53 AM
smh requested changes to this revision.EditedMar 1 2019, 9:23 AM

Sorry for lack of responses, I did see your requests but got lost in work stuff.

I've tested with zero for both standard ata and scsi ata pass through with no ill effects and as the ata spec defines the value as N/A I believe a zero is the more correct value.

If no objections I'll commit that change later today.

camcontrol.c
2295 ↗(On Diff #54563)

I believe 0 is actually the correct value in both case as both lba and count are ignored by the identify command.

This revision now requires changes to proceed.Mar 1 2019, 9:23 AM
danfe added inline comments.Mar 1 2019, 2:32 PM
camcontrol.c
2295 ↗(On Diff #54563)

I confirmed with the spec, it indeed specifies N/A, although I didn't find how they suggest N/A to be translated in implementations (code). That said, I'm fine with zero.

mav added a comment.Mar 1 2019, 2:38 PM

I agree, zero should be the right answer here. That is what kernel does also.

This revision was not accepted when it landed; it landed in state Needs Revision.Mar 1 2019, 2:39 PM
This revision was automatically updated to reflect the committed changes.
imp added a comment.Mar 1 2019, 4:34 PM

I figured that this was OK, but just checked this am and I noticed this commit.
Just wanted to say that it looks good to me too.
Thanks for driving this home @smh