netdev - Re: [PATCH 7/7] net/utils: Use strlcpy() instead of open-coding it

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <2C668AAE-BB6F-4292-84DD-25D4C6A5BC5F@intel.com>
Date:   Fri, 22 Mar 2019 18:46:31 +0000
From:   "Rustad, Mark D" <mark.d.rustad@...el.com>
To:     Bart Van Assche <bvanassche@....org>
CC:     David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Sagi Grimberg <sagi@...mberg.me>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 7/7] net/utils: Use strlcpy() instead of open-coding it

On Mar 21, 2019, at 3:19 PM, Bart Van Assche <bvanassche@....org> wrote:

> This patch does not change any functionality but makes the code easier
> to read.
>
> Cc: Sagi Grimberg <sagi@...mberg.me>
> Cc: Christoph Hellwig <hch@....de>
> Signed-off-by: Bart Van Assche <bvanassche@....org>
> ---
>  net/core/utils.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/net/core/utils.c b/net/core/utils.c
> index 2a597ac7808e..5f11e89f173f 100644
> --- a/net/core/utils.c
> +++ b/net/core/utils.c
> @@ -339,12 +339,8 @@ static int inet6_pton(struct net *net, const char  
> *src, u16 port_num,
>  	    src + srclen != scope_delim && *scope_delim == '%') {
>  		struct net_device *dev;
>  		char scope_id[16];
> -		size_t scope_len = min_t(size_t, sizeof(scope_id) - 1,
> -					 src + srclen - scope_delim - 1);
> -
> -		memcpy(scope_id, scope_delim + 1, scope_len);
> -		scope_id[scope_len] = '\0';
>
> +		strlcpy(scope_id, scope_delim + 1, sizeof(scope_id));
>  		dev = dev_get_by_name(net, scope_id);
>  		if (dev) {
>  			addr6->sin6_scope_id = dev->ifindex;
> --
> 2.21.0.155.ge902e9bcae20


This doesn't look right to me. The original code seemingly would stop the  
copy before getting to a possible '%' character. The new code only stops at  
eol. So this appears to change the functionality and I assume is broken in  
any case that uses the %. Take another look at what that min_t is really  
doing.

--
Mark Rustad, Networking Division, Intel Corporation

Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)