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.
We could check IEEE80211_MLD_MAX_NUM_LINKS instead of bits per long
if that's more readable?
regards,
dan carpenter
