Patchwork pci: aspm: fix memory read outside buffer

login
register
mail settings
Submitter Colin King
Date May 15, 2012, 6:03 p.m.
Message ID <1337105036-529-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/159405/
State Accepted
Headers show

Comments

Colin King - May 15, 2012, 6:03 p.m.
From: Colin Ian King <colin.king@canonical.com>

valgrind picked up the following bug:

==32563== Invalid read of size 1
==32563==    at 0x41DB6C: pcie_check_aspm_registers (aspm.c:247)
==32563==    by 0x534131C: fwts_framework_args (fwts_framework.c:608)
==32563==    by 0x403C68: main (main.c:27)
==32563==  Address 0x62e9ba3 is 2 bytes after a block of size 1 alloc'd
==32563==    at 0x4C29DB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32563==    by 0x5346F20: fwts_list_from_text (fwts_text_list.c:109)
==32563==    by 0x5345E94: fwts_pipe_exec (fwts_pipeio.c:149)
==32563==    by 0x41DB38: pcie_check_aspm_registers (aspm.c:236)
==32563==    by 0x534131C: fwts_framework_args (fwts_framework.c:608)
==32563==    by 0x403C68: main (main.c:27)

this occurs when parsing the output from lspci - the code is reading outside the
returned string and we need to check for short strings before parsing the line.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/pci/aspm/aspm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Alex Hung - May 16, 2012, 2:37 a.m.
On 05/16/2012 02:03 AM, Colin King wrote:
> From: Colin Ian King<colin.king@canonical.com>
>
> valgrind picked up the following bug:
>
> ==32563== Invalid read of size 1
> ==32563==    at 0x41DB6C: pcie_check_aspm_registers (aspm.c:247)
> ==32563==    by 0x534131C: fwts_framework_args (fwts_framework.c:608)
> ==32563==    by 0x403C68: main (main.c:27)
> ==32563==  Address 0x62e9ba3 is 2 bytes after a block of size 1 alloc'd
> ==32563==    at 0x4C29DB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==32563==    by 0x5346F20: fwts_list_from_text (fwts_text_list.c:109)
> ==32563==    by 0x5345E94: fwts_pipe_exec (fwts_pipeio.c:149)
> ==32563==    by 0x41DB38: pcie_check_aspm_registers (aspm.c:236)
> ==32563==    by 0x534131C: fwts_framework_args (fwts_framework.c:608)
> ==32563==    by 0x403C68: main (main.c:27)
>
> this occurs when parsing the output from lspci - the code is reading outside the
> returned string and we need to check for short strings before parsing the line.
>
> Signed-off-by: Colin Ian King<colin.king@canonical.com>
> ---
>   src/pci/aspm/aspm.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
> index c3c1c71..c461ce8 100644
> --- a/src/pci/aspm/aspm.c
> +++ b/src/pci/aspm/aspm.c
> @@ -244,7 +244,7 @@ int pcie_check_aspm_registers(fwts_framework *fw,
>   			char *line = fwts_text_list_text(item);
>   			char *pEnd;
>
> -			if (line[3] == ' ') {
> +			if (strlen(line)>= 3&&  line[3] == ' ') {
>   				reg_val = strtol(line,&pEnd, 16);
>   				for (i = 0; reg_loc<  256&&  i<  16; i++) {
>   					reg_val = strtol(pEnd + 1,&pEnd, 16);

Thanks for catching that.

Acked-by: Alex Hung <alex.hung@canonical.com>
Keng-Yu Lin - May 17, 2012, 6:11 a.m.
On Wed, May 16, 2012 at 10:37 AM, Alex Hung <alex.hung@canonical.com> wrote:
> On 05/16/2012 02:03 AM, Colin King wrote:
>>
>> From: Colin Ian King<colin.king@canonical.com>
>>
>> valgrind picked up the following bug:
>>
>> ==32563== Invalid read of size 1
>> ==32563==    at 0x41DB6C: pcie_check_aspm_registers (aspm.c:247)
>> ==32563==    by 0x534131C: fwts_framework_args (fwts_framework.c:608)
>> ==32563==    by 0x403C68: main (main.c:27)
>> ==32563==  Address 0x62e9ba3 is 2 bytes after a block of size 1 alloc'd
>> ==32563==    at 0x4C29DB4: calloc (in
>> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==32563==    by 0x5346F20: fwts_list_from_text (fwts_text_list.c:109)
>> ==32563==    by 0x5345E94: fwts_pipe_exec (fwts_pipeio.c:149)
>> ==32563==    by 0x41DB38: pcie_check_aspm_registers (aspm.c:236)
>> ==32563==    by 0x534131C: fwts_framework_args (fwts_framework.c:608)
>> ==32563==    by 0x403C68: main (main.c:27)
>>
>> this occurs when parsing the output from lspci - the code is reading
>> outside the
>> returned string and we need to check for short strings before parsing the
>> line.
>>
>> Signed-off-by: Colin Ian King<colin.king@canonical.com>
>> ---
>>  src/pci/aspm/aspm.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
>> index c3c1c71..c461ce8 100644
>> --- a/src/pci/aspm/aspm.c
>> +++ b/src/pci/aspm/aspm.c
>> @@ -244,7 +244,7 @@ int pcie_check_aspm_registers(fwts_framework *fw,
>>                        char *line = fwts_text_list_text(item);
>>                        char *pEnd;
>>
>> -                       if (line[3] == ' ') {
>> +                       if (strlen(line)>= 3&&  line[3] == ' ') {
>>                                reg_val = strtol(line,&pEnd, 16);
>>                                for (i = 0; reg_loc<  256&&  i<  16; i++) {
>>
>>                                        reg_val = strtol(pEnd + 1,&pEnd,
>> 16);
>
>
> Thanks for catching that.
>
> Acked-by: Alex Hung <alex.hung@canonical.com>
>
Acked-by: Keng-Yu Lin <kengyu@canonical.com>
Ivan Hu - May 17, 2012, 10:12 a.m.
On 05/17/2012 02:11 PM, Keng-Yu Lin wrote:
> On Wed, May 16, 2012 at 10:37 AM, Alex Hung<alex.hung@canonical.com>  wrote:
>> On 05/16/2012 02:03 AM, Colin King wrote:
>>> From: Colin Ian King<colin.king@canonical.com>
>>>
>>> valgrind picked up the following bug:
>>>
>>> ==32563== Invalid read of size 1
>>> ==32563==    at 0x41DB6C: pcie_check_aspm_registers (aspm.c:247)
>>> ==32563==    by 0x534131C: fwts_framework_args (fwts_framework.c:608)
>>> ==32563==    by 0x403C68: main (main.c:27)
>>> ==32563==  Address 0x62e9ba3 is 2 bytes after a block of size 1 alloc'd
>>> ==32563==    at 0x4C29DB4: calloc (in
>>> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>>> ==32563==    by 0x5346F20: fwts_list_from_text (fwts_text_list.c:109)
>>> ==32563==    by 0x5345E94: fwts_pipe_exec (fwts_pipeio.c:149)
>>> ==32563==    by 0x41DB38: pcie_check_aspm_registers (aspm.c:236)
>>> ==32563==    by 0x534131C: fwts_framework_args (fwts_framework.c:608)
>>> ==32563==    by 0x403C68: main (main.c:27)
>>>
>>> this occurs when parsing the output from lspci - the code is reading
>>> outside the
>>> returned string and we need to check for short strings before parsing the
>>> line.
>>>
>>> Signed-off-by: Colin Ian King<colin.king@canonical.com>
>>> ---
>>>   src/pci/aspm/aspm.c |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
>>> index c3c1c71..c461ce8 100644
>>> --- a/src/pci/aspm/aspm.c
>>> +++ b/src/pci/aspm/aspm.c
>>> @@ -244,7 +244,7 @@ int pcie_check_aspm_registers(fwts_framework *fw,
>>>                         char *line = fwts_text_list_text(item);
>>>                         char *pEnd;
>>>
>>> -                       if (line[3] == ' ') {
>>> +                       if (strlen(line)>= 3&&    line[3] == ' ') {
>>>                                 reg_val = strtol(line,&pEnd, 16);
>>>                                 for (i = 0; reg_loc<    256&&    i<    16; i++) {
>>>
>>>                                         reg_val = strtol(pEnd + 1,&pEnd,
>>> 16);
>>
>> Thanks for catching that.
>>
>> Acked-by: Alex Hung<alex.hung@canonical.com>
>>
> Acked-by: Keng-Yu Lin<kengyu@canonical.com>
>
Acked-by: Ivan Hu<ivan.hu@canonical.com>

Patch

diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
index c3c1c71..c461ce8 100644
--- a/src/pci/aspm/aspm.c
+++ b/src/pci/aspm/aspm.c
@@ -244,7 +244,7 @@  int pcie_check_aspm_registers(fwts_framework *fw,
 			char *line = fwts_text_list_text(item);
 			char *pEnd;
 
-			if (line[3] == ' ') {
+			if (strlen(line) >= 3 && line[3] == ' ') {
 				reg_val = strtol(line, &pEnd, 16);
 				for (i = 0; reg_loc < 256 && i < 16; i++) {
 					reg_val = strtol(pEnd + 1, &pEnd, 16);