On 12/07/2022 15:48, Paolo Bonzini wrote:
> Queued, thanks (I was on vacation last week).
>
> I am a bit scared about the mode_select_truncated quirk. My reading
> of the code is that the MODE SELECT would fail anyway because the
> page length does not match in scsi_disk_check_mode_select:
>
> len = mode_sense_page(s, page, &p, 0);
> if (len < 0 || len != expected_len) {
> return -1;
> }
>
> Is that correct? If not, I'm not sure where I am wrong. If so,
> I wonder if it is enough for the quirk to do just a "goto invalid_param;"
> in place of invalid_param_len.
(goes and looks)
The truncated MODE SELECT request in question seems to be only used by the initial
A/UX installation image and looks like this:
scsi_req_parsed target 3 lun 0 tag 0 command 21 dir 2 length 20
scsi_req_parsed_lba target 3 lun 0 tag 0 command 21 lba 0
scsi_req_alloc target 3 lun 0 tag 0
scsi_disk_new_request Command: lun=0 tag=0x0 data= 0x15 0x00 0x00 0x00 0x14 0x00
scsi_disk_emulate_command_MODE_SELECT Mode Select(6) (len 20)
scsi_req_continue target 3 lun 0 tag 0
scsi_disk_emulate_write_data Write buf_len=20
scsi_req_data target 3 lun 0 tag 0 len 20
scsi_req_continue target 3 lun 0 tag 0
This includes an 8 byte block descriptor which is used to set the CDROM sector size
to 512 bytes and so the request content looks like:
4 bytes hdr_len for MODE SELECT(6)
8 bytes bd_len for the block descriptor
8 bytes of data for page 0 (MODE_PAGE_R_W_ERROR) data with page_len = 10
Stepping through mode_select_pages() in the debugger shows that since page_len is set
correctly to 10 bytes (which matches the expected length in mode_sense_page()) the
above check succeeds.
I suspect what happened is that the original author built the MODE_PAGE_R_W_ERROR
page data correctly but miscalculated the length of the request in the CDB. Given
that the truncated request is seemingly accepted on real hardware, no-one actually
noticed until now :)
ATB,
Mark.