tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Implement mode_select for atapi tape drives

On Sat, 8 Aug 2009, Marc Balmer wrote:

The attached patch revision includes the change to use sizeof() in the calls to memset(), adds a KASSERT when loading the quirk data to ensure that the st->page_0_size is not larger than MAX_PAGE_0_SIZE, and adds the necessary quirk for my tape drive.

I like this. Maybe KASSERT that MAX_PAG_0_SIZE is <= sizeof(struct atapi_select) ? That could be combined into a single KASSERT, even.

Well, MAX_PAGE_0_SIZE affects only one member of the struct, not the whole thing.

I think there are two options to consider. One is to place the KASSERT() at the single place where st->page_0_size is set (where the quirk data is loaded). The alternative would be to put it in all the places where the value might cause a buffer overflow. In this case, we'd need

        atapi_select_len = 12 + st->page_0_size;
        KASSERT(atapi_select_len <= sizeof(atapi_select));

  and   scsi_select_len = 12 + st->page_0_size;
        KASSERT(scsi_select_len <= sizeof(scsi_select));

BTW, I really don't particularly care for the constant 12 in the calculation of the {atapi,scsi}_select_len values! In my opinion, we should rewrite these as

        atapi_select_len = sizeof(atapi_select.header) +
                           sizeof(atapi_select.blk_desc) +

Looks like the scsi code, in general, could probably use a major tune-up. :) (No, I'm not volunteering, at least not yet.)

|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:      |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul at   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at |
| Kernel Developer |                          | pgoyette at  |

Home | Main Index | Thread Index | Old Index