| 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
| ||
|
Message-ID: <1538043034.14416.42.camel@sipsolutions.net>
Date: Thu, 27 Sep 2018 12:10:34 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Cody Schuffelen <schuffelen@...gle.com>
Cc: Kalle Valo <kvalo@...eaurora.org>,
"David S . Miller" <davem@...emloft.net>,
linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org,
netdev@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH net-next v2] wireless-drivers: rtnetlink wifi simulation
device
On Wed, 2018-09-26 at 12:43 -0700, Cody Schuffelen wrote:
> ip link set eth0 down
> ip link set eth0 name buried_eth0
> ip link set buried_eth0 up
> ip link add link buried_eth0 name wlan0 type virt_wifi
>
> eth0 is renamed to buried_eth0 to avoid a network manager trying to
> manage it,
I feel you should remove this "buried" thing at least from the plain
instructions - first of all, I'm not convinced it actually *works* in
general (network managers are able to find it anyway, right? perhaps
android's has some rules?), and it's not really necessary from a kernel
POV.
Perhaps add a note like "you may have to rename or otherwise hide the
eth0 from your connection manager" (also "NetworkManager" is a specific
software, so "network manager" is confusing - I actually first thought
you meant NM).
Anyway, just a thought.
> Thanks for the detailed review! I believe I've addressed all comments.
Thanks!
> The problem we've experienced with routing the hwsim virtual medium from the
> inside the VM to outside the VM is this required running hwsim on the host
> kernel as well. This is too intrusive for our use-case, whereas wifi that
> exists completely inside the VM kernel is much more palatable.
Fair enough. I still think it'd be worthwhile in the long run because
then you could start multiple APs to simulate roaming etc. without
having to implement all of this here.
IOW - I don't think we should extend this much. As is, I think it's
fine, but I wouldn't want to see extensions like "have two scan result",
"let userspace control the RSSI of the scan results to force 'roaming'"
etc.
> +static struct ieee80211_channel channel_2ghz = {
> + .band = NL80211_BAND_2GHZ,
> + .center_freq = 5500,
> + .hw_value = 5500,
That doesn't seem right - a 2 GHz channel that's really 5.5 GHz?
> + .max_power = 5500,
That seems more like a copy/paste bug rather than intentional?
> +static struct ieee80211_rate bitrates_2ghz[] = {
> + {
> + .bitrate = 10,
> + }, {
> + .bitrate = 20,
> + }, {
> + .bitrate = 55,
> + }, {
> + .bitrate = 60,
> + }, {
> + .bitrate = 110,
> + }, {
> + .bitrate = 120,
> + }, {
> + .bitrate = 240,
> + },
> +};
That's ... strangely formatted? I guess we'd usually write
{ .bitrate = 10 },
{ .bitrate = 20 },
...
> +static struct ieee80211_supported_band band_2ghz = {
> + .channels = &channel_2ghz,
> + .bitrates = bitrates_2ghz,
> + .band = NL80211_BAND_2GHZ,
> + .n_channels = 1,
> + .n_bitrates = 7,
Please use ARRAY_SIZE()
> + .ht_cap = {
> + .ht_supported = true,
> + },
> + .vht_cap = {
> + .vht_supported = true,
> + },
That looks _really_ bare - you may want to copy something sane to here.
> +};
> +
> +static struct ieee80211_channel channel_5ghz = {
> + .max_power = 5500,
same here - max_power 5500?
> + .max_reg_power = 9999,
> +};
> +
> +static struct ieee80211_rate bitrates_5ghz[] = {
> + {
> + .bitrate = 60,
> + }, {
> + .bitrate = 120,
> + }, {
> + .bitrate = 240,
> + },
> +};
Same comment here regarding formatting.
> +static struct ieee80211_supported_band band_5ghz = {
> + .channels = &channel_5ghz,
> + .bitrates = bitrates_5ghz,
> + .band = NL80211_BAND_5GHZ,
> + .n_channels = 1,
> + .n_bitrates = 3,
and ARRAY_SIZE
> + .ht_cap = {
> + .ht_supported = true,
> + },
> + .vht_cap = {
> + .vht_supported = true,
> + },
and HT/VHT
> +/** Assigned at module init. Guaranteed locally-administered and unicast. */
> +static u8 fake_router_bssid[ETH_ALEN] = {};
Maybe make that __ro_after_init?
It doesn't matter that much, but is a good signal.
> +static void virt_wifi_scan_result(struct work_struct *work)
> +{
> + char ssid[] = "__VirtWifi";
> + struct cfg80211_bss *informed_bss;
> + struct virt_wifi_priv *priv =
> + container_of(work, struct virt_wifi_priv,
> + scan_result.work);
> + struct wiphy *wiphy = priv_to_wiphy(priv);
> + struct cfg80211_inform_bss mock_inform_bss = {
> + .chan = &channel_5ghz,
> + .scan_width = NL80211_BSS_CHAN_WIDTH_20,
> + .signal = -60,
> + .boottime_ns = ktime_get_boot_ns(),
> + };
> +
> + ssid[0] = WLAN_EID_SSID;
> + /* size of the array minus null terminator, length byte, tag byte */
> + ssid[1] = sizeof(ssid) - 3;
Seems like you could make that const?
struct {
u8 tag;
u8 len;
u8 ssid[8];
} ssid = { .tag = 0, .len = 8, .ssid = "VirtWifi" };
or something like that?
Hmm, maybe that doesn't work.. whatever, not really important.
> + informed_bss = cfg80211_inform_bss_data(wiphy, &mock_inform_bss,
> + CFG80211_BSS_FTYPE_PRESP,
> + fake_router_bssid,
> + mock_inform_bss.boottime_ns,
> + WLAN_CAPABILITY_ESS, 0, ssid,
> + /* Truncate before the null. */
> + sizeof(ssid) - 1, GFP_KERNEL);
> + cfg80211_put_bss(wiphy, informed_bss);
> +
> + informed_bss = cfg80211_inform_bss_data(wiphy, &mock_inform_bss,
> + CFG80211_BSS_FTYPE_BEACON,
> + fake_router_bssid,
> + mock_inform_bss.boottime_ns,
> + WLAN_CAPABILITY_ESS, 0, ssid,
> + /* Truncate before the null. */
> + sizeof(ssid) - 1, GFP_KERNEL);
> + cfg80211_put_bss(wiphy, informed_bss);
Hmm, what's the point of doing it twice? You don't really need to
receive both PRESP/BEACON, just one is sufficient.
> + schedule_delayed_work(&priv->scan_complete, HZ * 2);
> +}
I don't think you need to make that async again.
> +static int virt_wifi_connect(struct wiphy *wiphy, struct net_device *netdev,
> + struct cfg80211_connect_params *sme)
> +{
> + struct virt_wifi_priv *priv = wiphy_priv(wiphy);
> + bool could_schedule;
> +
> + if (priv->being_deleted)
> + return -EBUSY;
> +
> + if (sme->bssid && !ether_addr_equal(sme->bssid, fake_router_bssid))
> + return -EINVAL;
If you wanted to be more "real", you'd accept this and then check it in
the connect worker, rejecting it there if mismatched.
> +static int virt_wifi_get_station(struct wiphy *wiphy, struct net_device *dev,
> + const u8 *mac, struct station_info *sinfo)
> +{
> + wiphy_debug(wiphy, "get_station\n");
> + sinfo->filled = BIT(NL80211_STA_INFO_TX_PACKETS) |
> + BIT(NL80211_STA_INFO_TX_FAILED) | BIT(NL80211_STA_INFO_SIGNAL) |
> + BIT(NL80211_STA_INFO_TX_BITRATE);
> + sinfo->tx_packets = 1;
> + sinfo->tx_failed = 0;
> + sinfo->signal = -60;
> + sinfo->txrate = (struct rate_info) {
> + .legacy = 10, /* units are 100kbit/s */
> + };
> + return 0;
> +}
You should check that mac is fake_router_bssid, and otherwise return not
found (-ENOENT), IIRC.
> +static netdev_tx_t virt_wifi_start_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + struct virt_wifi_netdev_priv *priv = netdev_priv(dev);
> + struct virt_wifi_priv *w_priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
> +
> + if (!w_priv->is_connected)
> + return NETDEV_TX_BUSY;
I think you should just drop the frame.
> +/* Called under rcu_read_lock() from netif_receive_skb */
> +static rx_handler_result_t virt_wifi_rx_handler(struct sk_buff **pskb)
FWIW, the stuff beyond this point, especially the netlink, I'm less
familiar with.
> +/** Called with rtnl lock held. */
/** is usually used only for kernel-doc
johannes
Powered by blists - more mailing lists