* [PATCH 1/1] media: dib9000: avoid out of bound access
@ 2014-06-18 22:02 Heinrich Schuchardt
2014-06-18 23:50 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2014-06-18 22:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Michael Krufky, Kees Cook, linux-media, linux-kernel,
Heinrich Schuchardt
The current test to avoid out of bound access to mb[] is insufficient.
For len = 19 non-existent mb[10] will be accessed.
A check in the for loop is insufficient to avoid out of bound access in
dib9000_mbx_send_attr.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
drivers/media/dvb-frontends/dib9000.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/media/dvb-frontends/dib9000.c b/drivers/media/dvb-frontends/dib9000.c
index e540cfb..6a71917 100644
--- a/drivers/media/dvb-frontends/dib9000.c
+++ b/drivers/media/dvb-frontends/dib9000.c
@@ -1040,10 +1040,13 @@ static int dib9000_risc_apb_access_write(struct dib9000_state *state, u32 addres
if (address >= 1024 || !state->platform.risc.fw_is_running)
return -EINVAL;
+ if (len > 18)
+ return -EINVAL;
+
/* dprintk( "APB access thru wr fw %d %x", address, attribute); */
mb[0] = (unsigned short)address;
- for (i = 0; i < len && i < 20; i += 2)
+ for (i = 0; i < len; i += 2)
mb[1 + (i / 2)] = (b[i] << 8 | b[i + 1]);
dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len / 2, attribute);
--
2.0.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: dib9000: avoid out of bound access
2014-06-18 22:02 [PATCH 1/1] media: dib9000: avoid out of bound access Heinrich Schuchardt
@ 2014-06-18 23:50 ` Kees Cook
2014-06-19 1:41 ` Heinrich Schuchardt
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2014-06-18 23:50 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Mauro Carvalho Chehab, Michael Krufky, linux-media, LKML
On Wed, Jun 18, 2014 at 3:02 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> The current test to avoid out of bound access to mb[] is insufficient.
> For len = 19 non-existent mb[10] will be accessed.
>
> A check in the for loop is insufficient to avoid out of bound access in
> dib9000_mbx_send_attr.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> drivers/media/dvb-frontends/dib9000.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/dvb-frontends/dib9000.c b/drivers/media/dvb-frontends/dib9000.c
> index e540cfb..6a71917 100644
> --- a/drivers/media/dvb-frontends/dib9000.c
> +++ b/drivers/media/dvb-frontends/dib9000.c
> @@ -1040,10 +1040,13 @@ static int dib9000_risc_apb_access_write(struct dib9000_state *state, u32 addres
> if (address >= 1024 || !state->platform.risc.fw_is_running)
> return -EINVAL;
>
> + if (len > 18)
> + return -EINVAL;
> +
> /* dprintk( "APB access thru wr fw %d %x", address, attribute); */
>
> mb[0] = (unsigned short)address;
> - for (i = 0; i < len && i < 20; i += 2)
> + for (i = 0; i < len; i += 2)
> mb[1 + (i / 2)] = (b[i] << 8 | b[i + 1]);
Good catch on the mb[] access! However, I think there is still more to
fix since b[i + 1] can read past the end of b: Say b is defined as "u8
b[3]". Passing len 3 means the second loop, with i==2 will access b[2]
and b[3], the latter is out of range.
-Kees
>
> dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len / 2, attribute);
> --
> 2.0.0
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: dib9000: avoid out of bound access
2014-06-18 23:50 ` Kees Cook
@ 2014-06-19 1:41 ` Heinrich Schuchardt
2014-06-19 2:26 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2014-06-19 1:41 UTC (permalink / raw)
To: Kees Cook; +Cc: Mauro Carvalho Chehab, Michael Krufky, linux-media, LKML
On 19.06.2014 01:50, Kees Cook wrote:
> On Wed, Jun 18, 2014 at 3:02 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> The current test to avoid out of bound access to mb[] is insufficient.
>> For len = 19 non-existent mb[10] will be accessed.
>>
>> A check in the for loop is insufficient to avoid out of bound access in
>> dib9000_mbx_send_attr.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> drivers/media/dvb-frontends/dib9000.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/dvb-frontends/dib9000.c b/drivers/media/dvb-frontends/dib9000.c
>> index e540cfb..6a71917 100644
>> --- a/drivers/media/dvb-frontends/dib9000.c
>> +++ b/drivers/media/dvb-frontends/dib9000.c
>> @@ -1040,10 +1040,13 @@ static int dib9000_risc_apb_access_write(struct dib9000_state *state, u32 addres
>> if (address >= 1024 || !state->platform.risc.fw_is_running)
>> return -EINVAL;
>>
>> + if (len > 18)
>> + return -EINVAL;
>> +
>> /* dprintk( "APB access thru wr fw %d %x", address, attribute); */
>>
>> mb[0] = (unsigned short)address;
>> - for (i = 0; i < len && i < 20; i += 2)
>> + for (i = 0; i < len; i += 2)
>> mb[1 + (i / 2)] = (b[i] << 8 | b[i + 1]);
>
> Good catch on the mb[] access! However, I think there is still more to
> fix since b[i + 1] can read past the end of b: Say b is defined as "u8
> b[3]". Passing len 3 means the second loop, with i==2 will access b[2]
> and b[3], the latter is out of range.
b[] and len are provided by the caller of dib9000_risc_apb_access_write.
dib9000_risc_apb_access_write cannot verify if the length of b[] matches
len at all.
Currently dib9000_risc_apb_access_write cannot handle odd values of len.
This holds even true if b[] has been padded with zero to an even length:
For odd values of len the last byte is not passed to dib9000_mbx_send_attr.
What is left unclear is how odd values of len should be handled correctly:
Should the caller provide a b[] padded with 0 to the next even number of
bytes,
or should dib9000_risc_apb_access_write take care not to read more then
len bytes,
or should odd values of len cause an error EINVAL.
From what I read in the coding one source of the value of len is
tuner_attach(), which is called from outside the dib9000 driver.
Heinrich
>
> -Kees
>
>>
>> dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len / 2, attribute);
>> --
>> 2.0.0
>>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: dib9000: avoid out of bound access
2014-06-19 1:41 ` Heinrich Schuchardt
@ 2014-06-19 2:26 ` Kees Cook
2014-06-19 14:49 ` [PATCH 1/1 v2] " Heinrich Schuchardt
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2014-06-19 2:26 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Mauro Carvalho Chehab, Michael Krufky, linux-media, LKML
On Wed, Jun 18, 2014 at 6:41 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 19.06.2014 01:50, Kees Cook wrote:
>>
>> On Wed, Jun 18, 2014 at 3:02 PM, Heinrich Schuchardt <xypron.glpk@gmx.de>
>> wrote:
>>>
>>> The current test to avoid out of bound access to mb[] is insufficient.
>>> For len = 19 non-existent mb[10] will be accessed.
>>>
>>> A check in the for loop is insufficient to avoid out of bound access in
>>> dib9000_mbx_send_attr.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>> drivers/media/dvb-frontends/dib9000.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/dvb-frontends/dib9000.c
>>> b/drivers/media/dvb-frontends/dib9000.c
>>> index e540cfb..6a71917 100644
>>> --- a/drivers/media/dvb-frontends/dib9000.c
>>> +++ b/drivers/media/dvb-frontends/dib9000.c
>>> @@ -1040,10 +1040,13 @@ static int dib9000_risc_apb_access_write(struct
>>> dib9000_state *state, u32 addres
>>> if (address >= 1024 || !state->platform.risc.fw_is_running)
>>> return -EINVAL;
>>>
>>> + if (len > 18)
>>> + return -EINVAL;
>>> +
>>> /* dprintk( "APB access thru wr fw %d %x", address, attribute);
>>> */
>>>
>>> mb[0] = (unsigned short)address;
>>> - for (i = 0; i < len && i < 20; i += 2)
>>> + for (i = 0; i < len; i += 2)
>>> mb[1 + (i / 2)] = (b[i] << 8 | b[i + 1]);
>>
>>
>> Good catch on the mb[] access! However, I think there is still more to
>> fix since b[i + 1] can read past the end of b: Say b is defined as "u8
>> b[3]". Passing len 3 means the second loop, with i==2 will access b[2]
>> and b[3], the latter is out of range.
>
>
> b[] and len are provided by the caller of dib9000_risc_apb_access_write.
> dib9000_risc_apb_access_write cannot verify if the length of b[] matches len
> at all.
>
> Currently dib9000_risc_apb_access_write cannot handle odd values of len.
> This holds even true if b[] has been padded with zero to an even length: For
> odd values of len the last byte is not passed to dib9000_mbx_send_attr.
>
> What is left unclear is how odd values of len should be handled correctly:
>
> Should the caller provide a b[] padded with 0 to the next even number of
> bytes,
> or should dib9000_risc_apb_access_write take care not to read more then len
> bytes,
> or should odd values of len cause an error EINVAL.
>
> From what I read in the coding one source of the value of len is
> tuner_attach(), which is called from outside the dib9000 driver.
How about:
for (i = 0; i < len; i += 2) {
u16 val = b[i] << 8;
if (i + 1 < len)
val |= b[i + 1];
mb[1 + (i / 2)] = val;
That's defensive, and would have the same effect of callers doing the padding.
-Kees
>
> Heinrich
>
>
>>
>> -Kees
>>
>>>
>>> dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len /
>>> 2, attribute);
>>> --
>>> 2.0.0
>>>
>>
>>
>>
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1 v2] media: dib9000: avoid out of bound access
2014-06-19 2:26 ` Kees Cook
@ 2014-06-19 14:49 ` Heinrich Schuchardt
2014-06-19 16:34 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2014-06-19 14:49 UTC (permalink / raw)
To: Kees Cook
Cc: Mauro Carvalho Chehab, Michael Krufky, linux-media, linux-kernel,
Heinrich Schuchardt
This updated patch also fixes out of bound access to b[].
In dib9000_risc_apb_access_write() an out of bound access to mb[].
The current test to avoid out of bound access to mb[] is insufficient.
For len = 19 non-existent mb[10] will be accessed.
For odd values of len b[] is accessed out of bounds.
For large values of len an of bound access to mb[] may occur in
dib9000_mbx_send_attr.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
drivers/media/dvb-frontends/dib9000.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/media/dvb-frontends/dib9000.c b/drivers/media/dvb-frontends/dib9000.c
index e540cfb..f75dec4 100644
--- a/drivers/media/dvb-frontends/dib9000.c
+++ b/drivers/media/dvb-frontends/dib9000.c
@@ -1040,13 +1040,18 @@ static int dib9000_risc_apb_access_write(struct dib9000_state *state, u32 addres
if (address >= 1024 || !state->platform.risc.fw_is_running)
return -EINVAL;
+ if (len > 18)
+ return -EINVAL;
+
/* dprintk( "APB access thru wr fw %d %x", address, attribute); */
- mb[0] = (unsigned short)address;
- for (i = 0; i < len && i < 20; i += 2)
- mb[1 + (i / 2)] = (b[i] << 8 | b[i + 1]);
+ mb[0] = (u16)address;
+ for (i = 0; i + 1 < len; i += 2)
+ mb[1 + i / 2] = b[i] << 8 | b[i + 1];
+ if (len & 1)
+ mb[1 + len / 2] = b[len - 1] << 8;
- dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len / 2, attribute);
+ dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, (3 + len) / 2, attribute);
return dib9000_mbx_get_message_attr(state, IN_MSG_END_BRIDGE_APB_RW, mb, &s, attribute) == 1 ? 0 : -EINVAL;
}
--
2.0.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1 v2] media: dib9000: avoid out of bound access
2014-06-19 14:49 ` [PATCH 1/1 v2] " Heinrich Schuchardt
@ 2014-06-19 16:34 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2014-06-19 16:34 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Mauro Carvalho Chehab, Michael Krufky, linux-media, LKML
On Thu, Jun 19, 2014 at 7:49 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> This updated patch also fixes out of bound access to b[].
>
> In dib9000_risc_apb_access_write() an out of bound access to mb[].
>
> The current test to avoid out of bound access to mb[] is insufficient.
> For len = 19 non-existent mb[10] will be accessed.
>
> For odd values of len b[] is accessed out of bounds.
>
> For large values of len an of bound access to mb[] may occur in
> dib9000_mbx_send_attr.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> drivers/media/dvb-frontends/dib9000.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/dib9000.c b/drivers/media/dvb-frontends/dib9000.c
> index e540cfb..f75dec4 100644
> --- a/drivers/media/dvb-frontends/dib9000.c
> +++ b/drivers/media/dvb-frontends/dib9000.c
> @@ -1040,13 +1040,18 @@ static int dib9000_risc_apb_access_write(struct dib9000_state *state, u32 addres
> if (address >= 1024 || !state->platform.risc.fw_is_running)
> return -EINVAL;
>
> + if (len > 18)
> + return -EINVAL;
> +
> /* dprintk( "APB access thru wr fw %d %x", address, attribute); */
>
> - mb[0] = (unsigned short)address;
> - for (i = 0; i < len && i < 20; i += 2)
> - mb[1 + (i / 2)] = (b[i] << 8 | b[i + 1]);
> + mb[0] = (u16)address;
> + for (i = 0; i + 1 < len; i += 2)
> + mb[1 + i / 2] = b[i] << 8 | b[i + 1];
> + if (len & 1)
> + mb[1 + len / 2] = b[len - 1] << 8;
>
> - dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len / 2, attribute);
> + dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, (3 + len) / 2, attribute);
> return dib9000_mbx_get_message_attr(state, IN_MSG_END_BRIDGE_APB_RW, mb, &s, attribute) == 1 ? 0 : -EINVAL;
> }
>
> --
> 2.0.0
>
That looks great, thanks!
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-19 16:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-18 22:02 [PATCH 1/1] media: dib9000: avoid out of bound access Heinrich Schuchardt
2014-06-18 23:50 ` Kees Cook
2014-06-19 1:41 ` Heinrich Schuchardt
2014-06-19 2:26 ` Kees Cook
2014-06-19 14:49 ` [PATCH 1/1 v2] " Heinrich Schuchardt
2014-06-19 16:34 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).