Messages in this thread | | | Date | Tue, 07 Apr 2009 10:09:56 +0800 | From | Li Zefan <> | Subject | Re: [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. ) >
| |