Re: [PATCH] blktrace: fix pdu_len when tracing packet command requests

From: Li Zefan
Date: Mon Apr 06 2009 - 22:09:40 EST


Ingo Molnar wrote:
> * Li Zefan <lizf@xxxxxxxxxxxxxx> wrote:
>
>> Since commit d7e3c3249ef23b4617393c69fe464765b4ff1645 ("block: add
>> large command support"), struct request->cmd has been changed from
>> unsinged char cmd[BLK_MAX_CDB] to unsigned char *cmd.
>>
>> v1 -> v2:
>> - make sure rq->cmd_len is always intialized, and then we can use
>> rq->cmd_len instead of BLK_MAX_CDB.
>
> Thanks. I've added a 'v2-by: FUJITA Tomonori' and the Ack from
> Fujita-san as well to document the precise lineage of the fix.
>
> Note: there's an important robustness and security issue to check
> before we can apply this fully.
>
> variable-size records are always tricky and need a full audit of the
> software stack.
>
> rq->cmd_len comes from sg device ioctls, and the sg command header
> can have an arbitrary value for sg_io_v4::header_len. The only limit
> in the block layer at the moment is that it must fit into a single
> kmalloc() - and that - in theory - can be very large.
>
> So:
>
> 1) the ftrace ring-buffer code has to be checked (does it work well
> with larger than 4K records). Steve .. how well will it work?
>

There is a check:

ring_buffer_lock_reserve(length)
{
...
length = rb_calculate_event_length(length);
if (length > BUF_PAGE_SIZE)
goto out;
...
}

so if the record is around PAGE_SIZE, the event will not be recorded.

> 2) and the user-space blktrace+blkparse code has to be checked for
> overflows and static sizes as well. Jens, Alan?
>
> I had a quick look at the user-space code. It seems mostly fine.
> There appears to be one minor bug in blkrawverify.c:
>
> pdu_buf = malloc(bit->pdu_len);
> n = fread(pdu_buf, bit->pdu_len, 1, ifp);
> if (n == 0) {
> INC_BAD("bad pdu");
>
> malloc() can return NULL under memory pressure - shouldnt we
> check it for NULL instead of passing it to fread()?
>
> Oh, there does seem to be a buffer-overflow problem in
> blkparse_fmt.c:
>

and this can be fixed easily.

> static char *dump_pdu(unsigned char *pdu_buf, int pdu_len)
> {
> static char p[4096];
> int i, len;
>
> if (!pdu_buf || !pdu_len)
> return NULL;
>
> for (len = 0, i = 0; i < pdu_len; i++) {
> if (i)
> len += sprintf(p + len, " ");
>
> len += sprintf(p + len, "%02x", pdu_buf[i]);
> [...]
>
> that p[4096] is a buffer-overflow if the pdu_len goes over 4096.
> This is a small potential security issue if we apply this patch.
> Should be changed to malloc(pdu_len) instead.
>
> ( Relatively small because SG_IO ioctls are not normally allowed
> to unprivileged users so generating intentionally large packets
> to exploit a sysadmin running blkparse seems like a stretch of
> a threat model. )
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/