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 <[email protected]>
---
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
On Thu, 02 Apr 2009 10:17:26 +0800
Li Zefan <[email protected]> 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 <[email protected]>
> ---
> 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,
FUJITA Tomonori wrote:
> On Thu, 02 Apr 2009 10:17:26 +0800
> Li Zefan <[email protected]> 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 <[email protected]>
>> ---
>> 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,
>
>
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 <[email protected]>
---
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
* Li Zefan <[email protected]> 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
Commit-ID: e2494e1b42ebac402324105d57646489d19e2b01
Gitweb: http://git.kernel.org/tip/e2494e1b42ebac402324105d57646489d19e2b01
Author: Li Zefan <[email protected]>
AuthorDate: Thu, 2 Apr 2009 13:43:26 +0800
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
- 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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Jens Axboe <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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,
Ingo Molnar wrote:
> * Li Zefan <[email protected]> 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. )
>