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

Am 08.08.2009 um 16:02 schrieb Paul Goyette:

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) +

Oh, indeed.

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

haha.  thanks for volunteering ;)

- Marc

| 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