netdev - Re: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <52535E4E.2090804@windriver.com>
Date:	Tue, 8 Oct 2013 09:22:22 +0800
From:	Fan Du <fan.du@...driver.com>
To:	David Laight <David.Laight@...lab.com>
CC:	Sohny Thomas <sthomas@...ux.vnet.ibm.com>,
	<stephen@...workplumber.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer
 overflow warning

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html