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) +
st->page_0_size;