linux-kernel - Re: [PATCH next] wifi: rtw89: mcc: prevent shift wrapping in rtw89_core_mlsr

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 for Android: free password hash cracker in your pocket

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

Message-ID: <aDVtu6dpKfWOyBn6@stanley.mountain>
Date: Tue, 27 May 2025 10:46:03 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Zong-Zhe Yang <kevin_yang@...ltek.com>
Cc: Ping-Ke Shih <pkshih@...ltek.com>,
	"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH next] wifi: rtw89: mcc: prevent shift wrapping in
 rtw89_core_mlsr_switch()

On Tue, May 27, 2025 at 07:38:17AM +0000, Zong-Zhe Yang wrote:
> Dan Carpenter <dan.carpenter@...aro.org> wrote:
> > 
> > The "link_id" value comes from the user via debugfs.  If it's larger than BITS_PER_LONG then
> > that would result in shift wrapping and potentially an out of bounds access later.  Fortunately,
> > only root can write to debugfs files so the security impact is minimal.
> > 
> 
> Thank you for catching this problem.
> 
> > 
> > [...]
> > 
> > @@ -5239,6 +5239,9 @@ int rtw89_core_mlsr_switch(struct rtw89_dev *rtwdev, struct
> > rtw89_vif *rtwvif,
> >         if (unlikely(!ieee80211_vif_is_mld(vif)))
> >                 return -EOPNOTSUPP;
> > 
> > +       if (unlikely(link_id >= BITS_PER_LONG))
> > +               return -EINVAL;
> > +
> 
> Since I think this problem only comes from dbgfs path, would you like to just add a check in debug.c ?
> 
> For example,
> (based on 0 <= valid link id < IEEE80211_MLD_MAX_NUM_LINKS < BITS_PER_LONG)
> 
> rtw89_debug_priv_mlo_mode_set(...)
> {
>         ...
>         switch (mlo_mode) {
>         case RTW89_MLO_MODE_MLSR:
>                if (argv >= IEEE80211_MLD_MAX_NUM_LINKS)
>                        return -EINVAL;
>                 ...
> 

I'd prefer to add the check in one place instead of all the callers.
We could check IEEE80211_MLD_MAX_NUM_LINKS instead of bits per long
if that's more readable?

regards,
dan carpenter