Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Tue, May 27, 2025 at 07:38:17AM +0000, Zong-Zhe Yang wrote:
> > Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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.
