netdev - Re: [PATCH net-next] sockopt: Change getsockopt() of SO

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]

Date:	Thu, 08 Nov 2012 08:36:02 -0700
From:	Brian Haley <brian.haley@...com>
To:	David Miller <davem@...emloft.net>
CC:	Ben Hutchings <bhutchings@...arflare.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	Eric Dumazet <eric.dumazet@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE
 to return an interface name

On 11/05/2012 09:01 PM, Brian Haley wrote:
>>>>> +    rcu_read_lock();
>>>>> +    dev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if);
>>>>> +    if (dev)
>>>>> +        strcpy(devname, dev->name);
>>>>
>>>> This still races with the device name change, potentially providing
>>>> a name which never existed in the system, doesn't it?
>>>
>>> My only argument here is that SIOCGIFNAME has had this same code
>>> forever, and
>>> noone has ever complained about that returning a garbled name.  Even
>>> dev_get_by_name() only holds an rcu lock when doing a strncmp().
>>>
>>> We'd need to audit the whole kernel to catch all the places where we
>>> potentially
>>> look at dev->name while it could change.  Is it really worth it?
>>
>> A net device name can't be changed while the device is up, or while
>> another task holds the RTNL lock.  I think that covers almost all uses.
>> I don't know whether it's worth going out to look for exceptions, but we
>> might as well fix the cases we know about.
>
> So do you think we can fix these corner cases later and get the API
> right first?

Hi Dave,

I noticed this isn't in patchwork anymore.  Is it possible to get this 
in and then work on the other issue in a separate patch since it's not 
just this code that has this problem?

Of course I'm still not convinced that extra check is necessary (but 
I'll do it to make others happy) because if you do a setsockopt("eth0"), 
all you care is that a getsockopt() returns "eth0" - if it returns 
anything else, eth0_renamed, eth0_foo, etc you'll notice it's not the 
same and take action.  And the fact that the name can't change when UP 
means the odds are small it can happen anyways.

Thanks,

-Brian

--
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