diff mbox

net: don't call strlen() on the user buffer in packet_bind_spkt()

Message ID 1488289373.9415.251.camel@edumazet-glaptop3.roam.corp.google.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Feb. 28, 2017, 1:42 p.m. UTC 
On Tue, 2017-02-28 at 05:33 -0800, Eric Dumazet wrote:

> It looks a bug in this implementation of strlcpy() then.
> 

Apparently strlcpy(dest, src, size)  returns strlen(src), so we can not
use it in this context.

> sizeof(name) is 15.
> 
> If you use strncpy(X, uaddr->sa_data, 15) , then you might access
> uaddr->sa_data[14] and this would still be wrong, since sa_data has 14
> bytes only :
> 
> 
> struct sockaddr {
>   sa_family_t sa_family;
>   char        sa_data[14];
> };


Maybe then :

Comments

Alexander Potapenko Feb. 28, 2017, 1:47 p.m. UTC | #1 
On Tue, Feb 28, 2017 at 2:42 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-02-28 at 05:33 -0800, Eric Dumazet wrote:
>
>> It looks a bug in this implementation of strlcpy() then.
>>
>
> Apparently strlcpy(dest, src, size)  returns strlen(src), so we can not
> use it in this context.
Good point.
>> sizeof(name) is 15.
>>
>> If you use strncpy(X, uaddr->sa_data, 15) , then you might access
>> uaddr->sa_data[14] and this would still be wrong, since sa_data has 14
>> bytes only :
>>
>>
>> struct sockaddr {
>>   sa_family_t sa_family;
>>   char        sa_data[14];
>> };
>
>
> Maybe then :
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 2bd0d1949312c3d71c4b33529316dcfe76fa28f1..d2e7caa79d2604363316c7316864bed1f4971d29 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -3103,7 +3103,7 @@ static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
>                             int addr_len)
>  {
>         struct sock *sk = sock->sk;
> -       char name[15];
> +       char name[sizeof(uaddr->sa_data) + 1];
>
>         /*
>          *      Check legality
> @@ -3111,7 +3111,8 @@ static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
>
>         if (addr_len != sizeof(struct sockaddr))
>                 return -EINVAL;
> -       strlcpy(name, uaddr->sa_data, sizeof(name));
> +       memcpy(name, uaddr->sa_data, sizeof(uaddr->sa_data));
> +       name[sizeof(uaddr->sa_data)] = 0;
>
>         return packet_do_bind(sk, name, 0, pkt_sk(sk)->num);
>  }
>
SGTM. Shall I send the updated patch? (Sorry for asking, the patch
culture is hard)
>
Eric Dumazet Feb. 28, 2017, 1:54 p.m. UTC | #2 
On Tue, 2017-02-28 at 14:47 +0100, Alexander Potapenko wrote:

> SGTM. Shall I send the updated patch? (Sorry for asking, the patch
> culture is hard)

It is always nice to wait for feedback(s), before sending a v2, and a
v3, and a v4.

Sending 4 patches in one hour is considered not friendly ;)

So if I were you, I would wait a bit for additional comments before
sending a new version.

Thanks.
diff mbox

Patch

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2bd0d1949312c3d71c4b33529316dcfe76fa28f1..d2e7caa79d2604363316c7316864bed1f4971d29 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3103,7 +3103,7 @@  static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
 			    int addr_len)
 {
 	struct sock *sk = sock->sk;
-	char name[15];
+	char name[sizeof(uaddr->sa_data) + 1];
 
 	/*
 	 *	Check legality
@@ -3111,7 +3111,8 @@  static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
 
 	if (addr_len != sizeof(struct sockaddr))
 		return -EINVAL;
-	strlcpy(name, uaddr->sa_data, sizeof(name));
+	memcpy(name, uaddr->sa_data, sizeof(uaddr->sa_data));
+	name[sizeof(uaddr->sa_data)] = 0;
 
 	return packet_do_bind(sk, name, 0, pkt_sk(sk)->num);
 }