lkml.org 
[lkml]   [2009]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] blktrace: fix pdu_len when tracing packet command requests
Ingo Molnar wrote:
> * Li Zefan <lizf@cn.fujitsu.com> 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. )
>


\
 
 \ /
  Last update: 2009-04-07 04:11    [W:1.533 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site