* [PATCH] iproute2: xfrm state add abort issue
[not found] <524411AE.7000404@linux.vnet.ibm.com>
@ 2013-09-26 18:32 ` Sohny Thomas
2013-09-27 8:26 ` David Laight
0 siblings, 1 reply; 8+ messages in thread
From: Sohny Thomas @ 2013-09-26 18:32 UTC (permalink / raw)
To: stephen, netdev
ip xfrm state add causes a SIGABRT due to a strncpy_chk .
This happens since strncpy doesn't account for the '\0' .
I have fixed this using sizeof instead of strlen .
There is a redhat bug which documents this issue
https://bugzilla.redhat.com/show_bug.cgi?id=982761
Signed-off-by: Sohny Thomas <sohthoma@in.ibm.com>
--------------
diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index 389942c..7dd8799 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c
@@ -117,7 +117,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg,
enum xfrm_attr_type_t type,
char *name, char *key, char *buf, int max)
{
int len;
- int slen = strlen(key);
+ int slen = sizeof(key);
#if 0
/* XXX: verifying both name and key is required! */
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH] iproute2: xfrm state add abort issue
2013-09-26 18:32 ` [PATCH] iproute2: xfrm state add abort issue Sohny Thomas
@ 2013-09-27 8:26 ` David Laight
2013-09-28 13:24 ` Sohny Thomas
0 siblings, 1 reply; 8+ messages in thread
From: David Laight @ 2013-09-27 8:26 UTC (permalink / raw)
To: Sohny Thomas, stephen, netdev
> ip xfrm state add causes a SIGABRT due to a strncpy_chk .
> This happens since strncpy doesn't account for the '\0' .
> I have fixed this using sizeof instead of strlen .
>
> There is a redhat bug which documents this issue
>
> https://bugzilla.redhat.com/show_bug.cgi?id=982761
>
> Signed-off-by: Sohny Thomas <sohthoma@in.ibm.com>
>
> --------------
>
> diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
> index 389942c..7dd8799 100644
> --- a/ip/xfrm_state.c
> +++ b/ip/xfrm_state.c
> @@ -117,7 +117,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg,
> enum xfrm_attr_type_t type,
> char *name, char *key, char *buf, int max)
> {
> int len;
> - int slen = strlen(key);
> + int slen = sizeof(key);
you definitely don't want sizeof(key) - that is either 4 or 8.
David
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iproute2: xfrm state add abort issue
2013-09-27 8:26 ` David Laight
@ 2013-09-28 13:24 ` Sohny Thomas
2013-09-29 1:49 ` [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning Fan Du
0 siblings, 1 reply; 8+ messages in thread
From: Sohny Thomas @ 2013-09-28 13:24 UTC (permalink / raw)
To: David Laight; +Cc: stephen, netdev
On Friday 27 September 2013 01:56 PM, David Laight wrote:
>> ip xfrm state add causes a SIGABRT due to a strncpy_chk .
>> This happens since strncpy doesn't account for the '\0' .
>> I have fixed this using sizeof instead of strlen .
>>
>> There is a redhat bug which documents this issue
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=982761
>>
>> Signed-off-by: Sohny Thomas <sohthoma@in.ibm.com>
>>
>> --------------
>>
>> diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
>> index 389942c..7dd8799 100644
>> --- a/ip/xfrm_state.c
>> +++ b/ip/xfrm_state.c
>> @@ -117,7 +117,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg,
>> enum xfrm_attr_type_t type,
>> char *name, char *key, char *buf, int max)
>> {
>> int len;
>> - int slen = strlen(key);
>> + int slen = sizeof(key);
>
> you definitely don't want sizeof(key) - that is either 4 or 8.
oh damn my bad.
I think i will go with strlen(key) + 1.
or i will pass slen+1 to strncpy .
Regards,
Sohny
>
> David
>
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning
2013-09-28 13:24 ` Sohny Thomas
@ 2013-09-29 1:49 ` Fan Du
2013-09-29 3:21 ` Sohny Thomas
2013-10-01 4:10 ` Stephen Hemminger
0 siblings, 2 replies; 8+ messages in thread
From: Fan Du @ 2013-09-29 1:49 UTC (permalink / raw)
To: Sohny Thomas; +Cc: David Laight, stephen, netdev
On 2013年09月28日 21:24, Sohny Thomas wrote:
> On Friday 27 September 2013 01:56 PM, David Laight wrote:
>>> ip xfrm state add causes a SIGABRT due to a strncpy_chk .
>>> This happens since strncpy doesn't account for the '\0' .
>>> I have fixed this using sizeof instead of strlen .
>>>
>>> There is a redhat bug which documents this issue
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=982761
>>>
>>> Signed-off-by: Sohny Thomas <sohthoma@in.ibm.com>
>>>
>>> --------------
>>>
>>> diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
>>> index 389942c..7dd8799 100644
>>> --- a/ip/xfrm_state.c
>>> +++ b/ip/xfrm_state.c
>>> @@ -117,7 +117,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg,
>>> enum xfrm_attr_type_t type,
>>> char *name, char *key, char *buf, int max)
>>> {
>>> int len;
>>> - int slen = strlen(key);
>>> + int slen = sizeof(key);
>>
>> you definitely don't want sizeof(key) - that is either 4 or 8.
> oh damn my bad.
> I think i will go with strlen(key) + 1.
>
> or i will pass slen+1 to strncpy .
You must be kidding about this slen+1 or strlen(key) + 1 , right?
From 3895b3d88bcb873988089392079645f2c9665e95 Mon Sep 17 00:00:00 2001
From: Fan Du <fan.du@windriver.com>
Date: Sun, 29 Sep 2013 09:38:12 +0800
Subject: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer
overflow warning.
This bug is reported from below link:
https://bugzilla.redhat.com/show_bug.cgi?id=982761
An simplified command from its original reproducing method in bugzilla:
ip xfrm state add src 10.0.0.2 dst 10.0.0.1 proto ah spi 0x12345678 auth md5 12
will cause below spew from gcc:
*** buffer overflow detected ***: ./ip terminated
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x37)[0x7f894ae2b817]
/lib/x86_64-linux-gnu/libc.so.6(+0x109710)[0x7f894ae2a710]
/lib/x86_64-linux-gnu/libc.so.6(+0x1089f6)[0x7f894ae299f6]
./ip[0x42119d]
./ip(do_xfrm_state+0x231)[0x4217f1]
./ip[0x405d05]
./ip(main+0x2b4)[0x405934]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x7f894ad4276d]
./ip[0x405bb9]
======= Memory map: ========
00400000-0043c000 r-xp 00000000 08:01 1997183 /workbench/iproute2/ip/ip
0063b000-0063c000 r--p 0003b000 08:01 1997183 /workbench/iproute2/ip/ip
0063c000-00640000 rw-p 0003c000 08:01 1997183 /workbench/iproute2/ip/ip
00640000-00643000 rw-p 00000000 00:00 0
017c7000-017e8000 rw-p 00000000 00:00 0 [heap]
7f894ab0b000-7f894ab20000 r-xp 00000000 08:01 6032986 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f894ab20000-7f894ad1f000 ---p 00015000 08:01 6032986 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f894ad1f000-7f894ad20000 r--p 00014000 08:01 6032986 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f894ad20000-7f894ad21000 rw-p 00015000 08:01 6032986 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f894ad21000-7f894aed6000 r-xp 00000000 08:01 6033273 /lib/x86_64-linux-gnu/libc-2.15.so
7f894aed6000-7f894b0d5000 ---p 001b5000 08:01 6033273 /lib/x86_64-linux-gnu/libc-2.15.so
7f894b0d5000-7f894b0d9000 r--p 001b4000 08:01 6033273 /lib/x86_64-linux-gnu/libc-2.15.so
7f894b0d9000-7f894b0db000 rw-p 001b8000 08:01 6033273 /lib/x86_64-linux-gnu/libc-2.15.so
7f894b0db000-7f894b0e0000 rw-p 00000000 00:00 0
7f894b0e0000-7f894b0e2000 r-xp 00000000 08:01 6033272 /lib/x86_64-linux-gnu/libdl-2.15.so
7f894b0e2000-7f894b2e2000 ---p 00002000 08:01 6033272 /lib/x86_64-linux-gnu/libdl-2.15.so
7f894b2e2000-7f894b2e3000 r--p 00002000 08:01 6033272 /lib/x86_64-linux-gnu/libdl-2.15.so
7f894b2e3000-7f894b2e4000 rw-p 00003000 08:01 6033272 /lib/x86_64-linux-gnu/libdl-2.15.so
7f894b2e4000-7f894b306000 r-xp 00000000 08:01 6033287 /lib/x86_64-linux-gnu/ld-2.15.so
7f894b4e3000-7f894b4e6000 rw-p 00000000 00:00 0
7f894b503000-7f894b506000 rw-p 00000000 00:00 0
7f894b506000-7f894b507000 r--p 00022000 08:01 6033287 /lib/x86_64-linux-gnu/ld-2.15.so
7f894b507000-7f894b509000 rw-p 00023000 08:01 6033287 /lib/x86_64-linux-gnu/ld-2.15.so
7fff9a3c2000-7fff9a3e3000 rw-p 00000000 00:00 0 [stack]
7fff9a3ff000-7fff9a400000 r-xp 00000000 00:00 0 [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall]
This is a false positive warning as the destination pointer "buf" pointers to
an ZERO length array, which actually will occupy alg.buf mostly.
Fix this by using memcpy.
struct xfrm_algo {
char alg_name[64];
unsigned int alg_key_len; /* in bits */
char alg_key[0];
};
struct {
union {
struct xfrm_algo alg;
struct xfrm_algo_aead aead;
struct xfrm_algo_auth auth;
} u;
char buf[XFRM_ALGO_KEY_BUF_SIZE];
} alg = {};
buf = alg.u.alg.alg_key;
---
ip/xfrm_state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index 0d98e78..5cc87d3 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c
@@ -159,7 +159,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg, enum xfrm_attr_type_t type,
if (len > max)
invarg("\"ALGO-KEY\" makes buffer overflow\n", key);
- strncpy(buf, key, len);
+ memcpy(buf, key, len);
}
}
--
1.7.9.5
> Regards,
> Sohny
>>
>> David
>>
>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
浮沉随浪只记今朝笑
--fan
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning
2013-09-29 1:49 ` [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning Fan Du
@ 2013-09-29 3:21 ` Sohny Thomas
2013-09-30 9:29 ` David Laight
2013-10-01 4:10 ` Stephen Hemminger
1 sibling, 1 reply; 8+ messages in thread
From: Sohny Thomas @ 2013-09-29 3:21 UTC (permalink / raw)
To: Fan Du; +Cc: David Laight, stephen, netdev
On Sunday 29 September 2013 07:19 AM, Fan Du wrote:
>
>
> On 2013年09月28日 21:24, Sohny Thomas wrote:
>> On Friday 27 September 2013 01:56 PM, David Laight wrote:
>>>> ip xfrm state add causes a SIGABRT due to a strncpy_chk .
>>>> This happens since strncpy doesn't account for the '\0' .
>>>> I have fixed this using sizeof instead of strlen .
>>>>
>>>> There is a redhat bug which documents this issue
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=982761
>>>>
>>>> Signed-off-by: Sohny Thomas <sohthoma@in.ibm.com>
>>>>
>>>> --------------
>>>>
>>>> diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
>>>> index 389942c..7dd8799 100644
>>>> --- a/ip/xfrm_state.c
>>>> +++ b/ip/xfrm_state.c
>>>> @@ -117,7 +117,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg,
>>>> enum xfrm_attr_type_t type,
>>>> char *name, char *key, char *buf, int max)
>>>> {
>>>> int len;
>>>> - int slen = strlen(key);
>>>> + int slen = sizeof(key);
>>>
>>> you definitely don't want sizeof(key) - that is either 4 or 8.
>> oh damn my bad.
>> I think i will go with strlen(key) + 1.
>>
>> or i will pass slen+1 to strncpy .
>
> You must be kidding about this slen+1 or strlen(key) + 1 , right?
strncpy fails with an abort at strncpy_chk.
So I was assuming it was because of the '\0' , so the +1
Thanks for the below patch
>
>
>
> From 3895b3d88bcb873988089392079645f2c9665e95 Mon Sep 17 00:00:00 2001
> From: Fan Du <fan.du@windriver.com>
> Date: Sun, 29 Sep 2013 09:38:12 +0800
> Subject: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer
> overflow warning.
>
> This bug is reported from below link:
> https://bugzilla.redhat.com/show_bug.cgi?id=982761
>
> An simplified command from its original reproducing method in bugzilla:
> ip xfrm state add src 10.0.0.2 dst 10.0.0.1 proto ah spi 0x12345678 auth
> md5 12
> will cause below spew from gcc:
>
> *** buffer overflow detected ***: ./ip terminated
> ======= Backtrace: =========
> /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x37)[0x7f894ae2b817]
> /lib/x86_64-linux-gnu/libc.so.6(+0x109710)[0x7f894ae2a710]
> /lib/x86_64-linux-gnu/libc.so.6(+0x1089f6)[0x7f894ae299f6]
> ./ip[0x42119d]
> ./ip(do_xfrm_state+0x231)[0x4217f1]
> ./ip[0x405d05]
> ./ip(main+0x2b4)[0x405934]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x7f894ad4276d]
> ./ip[0x405bb9]
> ======= Memory map: ========
> 00400000-0043c000 r-xp 00000000 08:01 1997183
> /workbench/iproute2/ip/ip
> 0063b000-0063c000 r--p 0003b000 08:01 1997183
> /workbench/iproute2/ip/ip
> 0063c000-00640000 rw-p 0003c000 08:01 1997183
> /workbench/iproute2/ip/ip
> 00640000-00643000 rw-p 00000000 00:00 0
> 017c7000-017e8000 rw-p 00000000 00:00 0
> [heap]
> 7f894ab0b000-7f894ab20000 r-xp 00000000 08:01 6032986
> /lib/x86_64-linux-gnu/libgcc_s.so.1
> 7f894ab20000-7f894ad1f000 ---p 00015000 08:01 6032986
> /lib/x86_64-linux-gnu/libgcc_s.so.1
> 7f894ad1f000-7f894ad20000 r--p 00014000 08:01 6032986
> /lib/x86_64-linux-gnu/libgcc_s.so.1
> 7f894ad20000-7f894ad21000 rw-p 00015000 08:01 6032986
> /lib/x86_64-linux-gnu/libgcc_s.so.1
> 7f894ad21000-7f894aed6000 r-xp 00000000 08:01 6033273
> /lib/x86_64-linux-gnu/libc-2.15.so
> 7f894aed6000-7f894b0d5000 ---p 001b5000 08:01 6033273
> /lib/x86_64-linux-gnu/libc-2.15.so
> 7f894b0d5000-7f894b0d9000 r--p 001b4000 08:01 6033273
> /lib/x86_64-linux-gnu/libc-2.15.so
> 7f894b0d9000-7f894b0db000 rw-p 001b8000 08:01 6033273
> /lib/x86_64-linux-gnu/libc-2.15.so
> 7f894b0db000-7f894b0e0000 rw-p 00000000 00:00 0
> 7f894b0e0000-7f894b0e2000 r-xp 00000000 08:01 6033272
> /lib/x86_64-linux-gnu/libdl-2.15.so
> 7f894b0e2000-7f894b2e2000 ---p 00002000 08:01 6033272
> /lib/x86_64-linux-gnu/libdl-2.15.so
> 7f894b2e2000-7f894b2e3000 r--p 00002000 08:01 6033272
> /lib/x86_64-linux-gnu/libdl-2.15.so
> 7f894b2e3000-7f894b2e4000 rw-p 00003000 08:01 6033272
> /lib/x86_64-linux-gnu/libdl-2.15.so
> 7f894b2e4000-7f894b306000 r-xp 00000000 08:01 6033287
> /lib/x86_64-linux-gnu/ld-2.15.so
> 7f894b4e3000-7f894b4e6000 rw-p 00000000 00:00 0
> 7f894b503000-7f894b506000 rw-p 00000000 00:00 0
> 7f894b506000-7f894b507000 r--p 00022000 08:01 6033287
> /lib/x86_64-linux-gnu/ld-2.15.so
> 7f894b507000-7f894b509000 rw-p 00023000 08:01 6033287
> /lib/x86_64-linux-gnu/ld-2.15.so
> 7fff9a3c2000-7fff9a3e3000 rw-p 00000000 00:00 0
> [stack]
> 7fff9a3ff000-7fff9a400000 r-xp 00000000 00:00 0
> [vdso]
> ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0
> [vsyscall]
>
> This is a false positive warning as the destination pointer "buf"
> pointers to
> an ZERO length array, which actually will occupy alg.buf mostly.
> Fix this by using memcpy.
>
> struct xfrm_algo {
> char alg_name[64];
> unsigned int alg_key_len; /* in bits */
> char alg_key[0];
> };
>
> struct {
> union {
> struct xfrm_algo alg;
> struct xfrm_algo_aead aead;
> struct xfrm_algo_auth auth;
> } u;
> char buf[XFRM_ALGO_KEY_BUF_SIZE];
> } alg = {};
>
> buf = alg.u.alg.alg_key;
> ---
> ip/xfrm_state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
> index 0d98e78..5cc87d3 100644
> --- a/ip/xfrm_state.c
> +++ b/ip/xfrm_state.c
> @@ -159,7 +159,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg,
> enum xfrm_attr_type_t type,
> if (len > max)
> invarg("\"ALGO-KEY\" makes buffer overflow\n", key);
>
> - strncpy(buf, key, len);
> + memcpy(buf, key, len);
> }
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning
2013-09-29 3:21 ` Sohny Thomas
@ 2013-09-30 9:29 ` David Laight
2013-10-08 1:22 ` Fan Du
0 siblings, 1 reply; 8+ messages in thread
From: David Laight @ 2013-09-30 9:29 UTC (permalink / raw)
To: Sohny Thomas, Fan Du; +Cc: stephen, netdev
> > This is a false positive warning as the destination pointer "buf"
> > pointers to
> > an ZERO length array, which actually will occupy alg.buf mostly.
> > Fix this by using memcpy.
> >
> > struct xfrm_algo {
> > char alg_name[64];
> > unsigned int alg_key_len; /* in bits */
> > char alg_key[0];
> > };
> >
> > struct {
> > union {
> > struct xfrm_algo alg;
> > struct xfrm_algo_aead aead;
> > struct xfrm_algo_auth auth;
> > } u;
> > char buf[XFRM_ALGO_KEY_BUF_SIZE];
> > } alg = {};
> >
> > buf = alg.u.alg.alg_key;
That is worse than horrid...
The tools have every right to complain about any accesses to alg_key[].
> > ---
> > ip/xfrm_state.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
> > index 0d98e78..5cc87d3 100644
> > --- a/ip/xfrm_state.c
> > +++ b/ip/xfrm_state.c
> > @@ -159,7 +159,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg,
> > enum xfrm_attr_type_t type,
> > if (len > max)
> > invarg("\"ALGO-KEY\" makes buffer overflow\n", key);
I presume there is a return hiding in invarg().
> >
> > - strncpy(buf, key, len);
> > + memcpy(buf, key, len);
Passing the length of the SOURCE to strncpy() is almost always wrong.
You are still not terminating the copied string.
David
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning
2013-09-29 1:49 ` [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning Fan Du
2013-09-29 3:21 ` Sohny Thomas
@ 2013-10-01 4:10 ` Stephen Hemminger
1 sibling, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2013-10-01 4:10 UTC (permalink / raw)
To: Fan Du; +Cc: Sohny Thomas, David Laight, netdev
> diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
> index 0d98e78..5cc87d3 100644
> --- a/ip/xfrm_state.c
> +++ b/ip/xfrm_state.c
> @@ -159,7 +159,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg, enum xfrm_attr_type_t type,
> if (len > max)
> invarg("\"ALGO-KEY\" makes buffer overflow\n", key);
>
> - strncpy(buf, key, len);
> + memcpy(buf, key, len);
> }
> }
>
Applied this patch. With some edits to make commit message logical.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning
2013-09-30 9:29 ` David Laight
@ 2013-10-08 1:22 ` Fan Du
0 siblings, 0 replies; 8+ messages in thread
From: Fan Du @ 2013-10-08 1:22 UTC (permalink / raw)
To: David Laight; +Cc: Sohny Thomas, stephen, netdev
Back from vacation, sorry for the late reply.
On 2013年09月30日 17:29, David Laight wrote:
>>> This is a false positive warning as the destination pointer "buf"
>>> pointers to
>>> an ZERO length array, which actually will occupy alg.buf mostly.
>>> Fix this by using memcpy.
>>>
>>> struct xfrm_algo {
>>> char alg_name[64];
>>> unsigned int alg_key_len; /* in bits */
>>> char alg_key[0];
>>> };
>>>
>>> struct {
>>> union {
>>> struct xfrm_algo alg;
>>> struct xfrm_algo_aead aead;
>>> struct xfrm_algo_auth auth;
>>> } u;
>>> char buf[XFRM_ALGO_KEY_BUF_SIZE];
>>> } alg = {};
>>>
>>> buf = alg.u.alg.alg_key;
>
> That is worse than horrid...
> The tools have every right to complain about any accesses to alg_key[].
Only when using strcpy, because a build in checking inserted in this function.
>>> ---
>>> ip/xfrm_state.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
>>> index 0d98e78..5cc87d3 100644
>>> --- a/ip/xfrm_state.c
>>> +++ b/ip/xfrm_state.c
>>> @@ -159,7 +159,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg,
>>> enum xfrm_attr_type_t type,
>>> if (len> max)
>>> invarg("\"ALGO-KEY\" makes buffer overflow\n", key);
>
> I presume there is a return hiding in invarg().
Good guess :)
>>>
>>> - strncpy(buf, key, len);
>>> + memcpy(buf, key, len);
>
> Passing the length of the SOURCE to strncpy() is almost always wrong.
> You are still not terminating the copied string.
Don't worry.
The length using here has been increased by 1 at the beginning of the function,
so the copied string to the destination is terminated well.
> David
>
>
--
浮沉随浪只记今朝笑
--fan
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-10-08 1:22 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <524411AE.7000404@linux.vnet.ibm.com>
2013-09-26 18:32 ` [PATCH] iproute2: xfrm state add abort issue Sohny Thomas
2013-09-27 8:26 ` David Laight
2013-09-28 13:24 ` Sohny Thomas
2013-09-29 1:49 ` [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning Fan Du
2013-09-29 3:21 ` Sohny Thomas
2013-09-30 9:29 ` David Laight
2013-10-08 1:22 ` Fan Du
2013-10-01 4:10 ` Stephen Hemminger
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).