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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <c70e23c6e9134f7e8b8791b5ece6baa3@realtek.com>
Date: Tue, 27 May 2025 07:58:13 +0000
From: Zong-Zhe Yang <kevin_yang@...ltek.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
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()

Dan Carpenter <dan.carpenter@...aro.org> wrote:
> 
> 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.

Understandable.

> We could check IEEE80211_MLD_MAX_NUM_LINKS instead of bits per long if that's more
> readable?

Sound good to me.