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

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

[Li Zefan]

This bug has been here since 2.6.26.

=======

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.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/blktrace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index fd1ff49..bafb8d3 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -642,7 +642,7 @@ static void blk_add_trace_rq(struct request_queue *q, struct request *rq,
 	if (blk_pc_request(rq)) {
 		what |= BLK_TC_ACT(BLK_TC_PC);
 		__blk_add_trace(bt, 0, rq->data_len, rw, what, rq->errors,
-				sizeof(rq->cmd), rq->cmd);
+				max_t(int, rq->cmd_len, BLK_MAX_CDB), rq->cmd);
 	} else  {
 		what |= BLK_TC_ACT(BLK_TC_FS);
 		__blk_add_trace(bt, rq->hard_sector, rq->hard_nr_sectors << 9,
-- 
1.5.4.rc3

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

[FUJITA Tomonori]

On Thu, 02 Apr 2009 10:17:26 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:

> This bug has been here since 2.6.26.
> 
> =======
> 
> 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.
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/trace/blktrace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index fd1ff49..bafb8d3 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -642,7 +642,7 @@ static void blk_add_trace_rq(struct request_queue *q, struct request *rq,
>  	if (blk_pc_request(rq)) {
>  		what |= BLK_TC_ACT(BLK_TC_PC);
>  		__blk_add_trace(bt, 0, rq->data_len, rw, what, rq->errors,
> -				sizeof(rq->cmd), rq->cmd);
> +				max_t(int, rq->cmd_len, BLK_MAX_CDB), rq->cmd);
>  	} else  {
>  		what |= BLK_TC_ACT(BLK_TC_FS);
>  		__blk_add_trace(bt, rq->hard_sector, rq->hard_nr_sectors << 9,
> -- 

The patch works however it would be cleaner to simply use cmd_len?

diff --git a/block/blk-core.c b/block/blk-core.c
index 996ed90..54765d7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -132,6 +132,7 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 	INIT_HLIST_NODE(&rq->hash);
 	RB_CLEAR_NODE(&rq->rb_node);
 	rq->cmd = rq->__cmd;
+	rq->cmd_len = BLK_MAX_CDB;
 	rq->tag = -1;
 	rq->ref_count = 1;
 }
diff --git a/block/blktrace.c b/block/blktrace.c
index 028120a..90b9ca4 100644
--- a/block/blktrace.c
+++ b/block/blktrace.c
@@ -583,7 +583,7 @@ static void blk_add_trace_rq(struct request_queue *q, struct request *rq,
 	if (blk_pc_request(rq)) {
 		what |= BLK_TC_ACT(BLK_TC_PC);
 		__blk_add_trace(bt, 0, rq->data_len, rw, what, rq->errors,
-				sizeof(rq->cmd), rq->cmd);
+				rq->cmd_len, rq->cmd);
 	} else  {
 		what |= BLK_TC_ACT(BLK_TC_FS);
 		__blk_add_trace(bt, rq->hard_sector, rq->hard_nr_sectors << 9,

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

[Li Zefan]

FUJITA Tomonori wrote:
> On Thu, 02 Apr 2009 10:17:26 +0800
> Li Zefan <lizf@cn.fujitsu.com> wrote:
> 
>> This bug has been here since 2.6.26.
>>
>> =======
>>
>> 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.
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  kernel/trace/blktrace.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
>> index fd1ff49..bafb8d3 100644
>> --- a/kernel/trace/blktrace.c
>> +++ b/kernel/trace/blktrace.c
>> @@ -642,7 +642,7 @@ static void blk_add_trace_rq(struct request_queue *q, struct request *rq,
>>  	if (blk_pc_request(rq)) {
>>  		what |= BLK_TC_ACT(BLK_TC_PC);
>>  		__blk_add_trace(bt, 0, rq->data_len, rw, what, rq->errors,
>> -				sizeof(rq->cmd), rq->cmd);
>> +				max_t(int, rq->cmd_len, BLK_MAX_CDB), rq->cmd);
>>  	} else  {
>>  		what |= BLK_TC_ACT(BLK_TC_FS);
>>  		__blk_add_trace(bt, rq->hard_sector, rq->hard_nr_sectors << 9,
>> -- 
> 
> The patch works however it would be cleaner to simply use cmd_len?
> 

Right! This looks better, thanks.

> diff --git a/block/blk-core.c b/block/blk-core.c
> index 996ed90..54765d7 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -132,6 +132,7 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
>  	INIT_HLIST_NODE(&rq->hash);
>  	RB_CLEAR_NODE(&rq->rb_node);
>  	rq->cmd = rq->__cmd;
> +	rq->cmd_len = BLK_MAX_CDB;
>  	rq->tag = -1;
>  	rq->ref_count = 1;
>  }
> diff --git a/block/blktrace.c b/block/blktrace.c
> index 028120a..90b9ca4 100644
> --- a/block/blktrace.c
> +++ b/block/blktrace.c
> @@ -583,7 +583,7 @@ static void blk_add_trace_rq(struct request_queue *q, struct request *rq,
>  	if (blk_pc_request(rq)) {
>  		what |= BLK_TC_ACT(BLK_TC_PC);
>  		__blk_add_trace(bt, 0, rq->data_len, rw, what, rq->errors,
> -				sizeof(rq->cmd), rq->cmd);
> +				rq->cmd_len, rq->cmd);
>  	} else  {
>  		what |= BLK_TC_ACT(BLK_TC_FS);
>  		__blk_add_trace(bt, rq->hard_sector, rq->hard_nr_sectors << 9,
> 
> 

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

[Li Zefan]

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.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 block/blk-core.c        |    1 +
 kernel/trace/blktrace.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 29bcfac..859879d 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -132,6 +132,7 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 	INIT_HLIST_NODE(&rq->hash);
 	RB_CLEAR_NODE(&rq->rb_node);
 	rq->cmd = rq->__cmd;
+	rq->cmd_len = BLK_MAX_CDB;
 	rq->tag = -1;
 	rq->ref_count = 1;
 }
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 947c5b3..cc6a9c2 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -642,7 +642,7 @@ static void blk_add_trace_rq(struct request_queue *q, struct request *rq,
 	if (blk_pc_request(rq)) {
 		what |= BLK_TC_ACT(BLK_TC_PC);
 		__blk_add_trace(bt, 0, rq->data_len, rw, what, rq->errors,
-				sizeof(rq->cmd), rq->cmd);
+				rq->cmd_len, rq->cmd);
 	} else  {
 		what |= BLK_TC_ACT(BLK_TC_FS);
 		__blk_add_trace(bt, rq->hard_sector, rq->hard_nr_sectors << 9,
-- 
1.5.4.rc3

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

[Ingo Molnar]

* 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?

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:

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

Anyway, this needs to be fixed and fully audited, as we can 
literally get a 128K packet traced here - even though the hardware 
itself wont be able to do much with it - most packet commands are in 
the few bytes range up to 16 bytes typically - but the blktrace 
layer will forward it.

Please double-check that blkparse is not surprised by the 
(now-again-) variable length packet command output either.

>From v2.6.26 on we only emitted the first 4/8 bytes depending on 
bitness.

Thanks,

	Ingo

[tip:tracing/blktrace-v2] blktrace: fix pdu_len when tracing packet command requests

[Li Zefan]

Commit-ID:  e2494e1b42ebac402324105d57646489d19e2b01
Gitweb:     http://git.kernel.org/tip/e2494e1b42ebac402324105d57646489d19e2b01
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Thu, 2 Apr 2009 13:43:26 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 3 Apr 2009 15:29:26 +0200

blktrace: fix pdu_len when tracing packet command requests

Impact: output all of packet commands - not just the first 4 / 8 bytes

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: by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

- make sure rq->cmd_len is always intialized, and then we can use
  rq->cmd_len instead of BLK_MAX_CDB.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
LKML-Reference: <49D4507E.2060602@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 block/blk-core.c        |    1 +
 kernel/trace/blktrace.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 29bcfac..859879d 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -132,6 +132,7 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 	INIT_HLIST_NODE(&rq->hash);
 	RB_CLEAR_NODE(&rq->rb_node);
 	rq->cmd = rq->__cmd;
+	rq->cmd_len = BLK_MAX_CDB;
 	rq->tag = -1;
 	rq->ref_count = 1;
 }
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 73d7860..b32ff44 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -643,7 +643,7 @@ static void blk_add_trace_rq(struct request_queue *q, struct request *rq,
 	if (blk_pc_request(rq)) {
 		what |= BLK_TC_ACT(BLK_TC_PC);
 		__blk_add_trace(bt, 0, rq->data_len, rw, what, rq->errors,
-				sizeof(rq->cmd), rq->cmd);
+				rq->cmd_len, rq->cmd);
 	} else  {
 		what |= BLK_TC_ACT(BLK_TC_FS);
 		__blk_add_trace(bt, rq->hard_sector, rq->hard_nr_sectors << 9,

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

[Li Zefan]

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