Re: [PATCH v3 00/14] scsi: add quirks and features to support m68k Macs

[Mark Cave-Ayland]

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.