| 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: <CAL+tcoB8kP-q-RsWBrow0yauGd5aiqq=zmokD-s5GWrqOZx=EA@mail.gmail.com>
Date: Mon, 19 May 2025 08:19:19 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Bert Karwatzki <spasswolf@....de>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-next@...r.kernel.org, Johannes Berg <johannes.berg@...el.com>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] wifi: Check if socket flags are valid
On Mon, May 19, 2025 at 12:46 AM Bert Karwatzki <spasswolf@....de> wrote:
>
> The check, if a particular SO_* flag_bit is set, may give a wrong result
> since sk_flags are part of a union and the union is used otherwise.
> This happens, if a socket is not a full socket, like a request socket
> for example.
>
> Add a check to verify, if the union is used for sk_flags.
>
> This solution is taken from commit
> e8a64bbaaad1 ("net/sched: taprio: Check if socket flags are valid").
>
> Fixes: 76a853f86c97 ("wifi: free SKBTX_WIFI_STATUS skb tx_flags flag")
>
nit: empty line
> Signed-off-by: Bert Karwatzki <spasswolf@....de>
Great! Thanks!
Reviewed-by: Jason Xing <kerneljasonxing@...il.com>
> ---
> drivers/net/wireless/ath/wil6210/txrx.h | 2 +-
> drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
> net/mac80211/mesh.c | 2 +-
> net/mac80211/tx.c | 6 +++---
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/txrx.h b/drivers/net/wireless/ath/wil6210/txrx.h
> index 33ccd0b248d4..91432b318ec2 100644
> --- a/drivers/net/wireless/ath/wil6210/txrx.h
> +++ b/drivers/net/wireless/ath/wil6210/txrx.h
> @@ -618,7 +618,7 @@ static inline bool wil_need_txstat(struct sk_buff *skb)
> const u8 *da = wil_skb_get_da(skb);
>
> return is_unicast_ether_addr(da) && skb->sk &&
> - sock_flag(skb->sk, SOCK_WIFI_STATUS);
> + sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS);
> }
>
> static inline void wil_consume_skb(struct sk_buff *skb, bool acked)
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 1485f949ad4e..973df2656238 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -913,7 +913,7 @@ mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
>
> multicast = is_multicast_ether_addr(skb->data);
>
> - if (unlikely(!multicast && skb->sk &&
> + if (unlikely(!multicast && skb->sk && sk_fullsock(skb->sk) &&
> sock_flag(skb->sk, SOCK_WIFI_STATUS) &&
> priv->adapter->fw_api_ver == MWIFIEX_FW_V15))
> skb = mwifiex_clone_skb_for_tx_status(priv,
> diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
> index a381b4b756ea..11b6cb639ae7 100644
> --- a/net/mac80211/mesh.c
> +++ b/net/mac80211/mesh.c
> @@ -777,7 +777,7 @@ bool ieee80211_mesh_xmit_fast(struct ieee80211_sub_if_data *sdata,
> if (ethertype < ETH_P_802_3_MIN)
> return false;
>
> - if (skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS))
> + if (skb->sk && sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS))
> return false;
>
> if (skb->ip_summed == CHECKSUM_PARTIAL) {
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 3b9392a6ddb2..8b5bcddd5cc9 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -2859,7 +2859,7 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
> }
>
> if (unlikely(!multicast &&
> - ((skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS)) ||
> + ((skb->sk && sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS)) ||
> ctrl_flags & IEEE80211_TX_CTL_REQ_TX_STATUS)))
> info_id = ieee80211_store_ack_skb(local, skb, &info_flags,
> cookie);
> @@ -3756,7 +3756,7 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata,
> return false;
>
> /* don't handle TX status request here either */
> - if (skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS))
> + if (skb->sk && sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS))
> return false;
>
> if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) {
> @@ -4648,7 +4648,7 @@ static void ieee80211_8023_xmit(struct ieee80211_sub_if_data *sdata,
> memcpy(IEEE80211_SKB_CB(seg), info, sizeof(*info));
> }
>
> - if (unlikely(skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS))) {
> + if (unlikely(skb->sk && sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS))) {
> info->status_data = ieee80211_store_ack_skb(local, skb,
> &info->flags, NULL);
> if (info->status_data)
> --
> 2.49.0
>
> I've dug a little into the history of sk_flags (which have been introduced in
> v4.4) and found commit e8a64bbaaad1 ("net/sched: taprio: Check if socket flags
> are valid"), which seems to address the same problem we're currently facing:
Indeed, sk_flags can only be used by a full socket.
Thanks,
Jason
>
> commit e8a64bbaaad1f6548cec5508297bc6d45e8ab69e
> Author: Benedikt Spranger <b.spranger@...utronix.de>
> Date: Fri Apr 8 11:47:45 2022 +0200
>
> net/sched: taprio: Check if socket flags are valid
>
> A user may set the SO_TXTIME socket option to ensure a packet is send
> at a given time. The taprio scheduler has to confirm, that it is allowed
> to send a packet at that given time, by a check against the packet time
> schedule. The scheduler drop the packet, if the gates are closed at the
> given send time.
>
> The check, if SO_TXTIME is set, may fail since sk_flags are part of an
> union and the union is used otherwise. This happen, if a socket is not
> a full socket, like a request socket for example.
>
> Add a check to verify, if the union is used for sk_flags.
>
> Fixes: 4cfd5779bd6e ("taprio: Add support for txtime-assist mode")
> Signed-off-by: Benedikt Spranger <b.spranger@...utronix.de>
> Reviewed-by: Kurt Kanzenbach <kurt@...utronix.de>
> Acked-by: Vinicius Costa Gomes <vinicius.gomes@...el.com>
> Signed-off-by: David S. Miller <davem@...emloft.net>
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 377f896bdedc..b9c71a304d39 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -417,7 +417,8 @@ static int taprio_enqueue_one(struct sk_buff *skb, struct Qdisc *sch,
> {
> struct taprio_sched *q = qdisc_priv(sch);
>
> - if (skb->sk && sock_flag(skb->sk, SOCK_TXTIME)) {
> + /* sk_flags are only safe to use on full sockets. */
> + if (skb->sk && sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_TXTIME)) {
> if (!is_valid_interval(skb, sch))
> return qdisc_drop(skb, sch, to_free);
> } else if (TXTIME_ASSIST_IS_ENABLED(q->flags)) {
>
> I'm not sure if all sk_fullsock() checks are necessary, or if it can
> be guessed from context if the socket is valid, though.
>
> This has been tested for ~1h so far.
>
> Bert Karwatzki
Powered by blists - more mailing lists