[v1] wifi: rtw89: mcc: prevent shift wrapping in rtw89_core_mlsr_switch()

[PATCH next] wifi: rtw89: mcc: prevent shift wrapping in rtw89_core_mlsr_switch()

Posted by Dan Carpenter 6 months, 3 weeks ago

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.

Fixes: 9dd85e739ce0 ("wifi: rtw89: debug: add mlo_mode dbgfs")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
---
 drivers/net/wireless/realtek/rtw89/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 49447668cbf3..7e5f87700941 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -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;
+
 	if (unlikely(!(usable_links & BIT(link_id)))) {
 		rtw89_warn(rtwdev, "%s: link id %u is not usable\n", __func__,
 			   link_id);
-- 
2.47.2

RE: [PATCH next] wifi: rtw89: mcc: prevent shift wrapping in rtw89_core_mlsr_switch()

Posted by Zong-Zhe Yang 6 months, 3 weeks ago

Dan Carpenter <dan.carpenter@linaro.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;
                ...


>         if (unlikely(!(usable_links & BIT(link_id)))) {
>                 rtw89_warn(rtwdev, "%s: link id %u is not usable\n", __func__,
>                            link_id);
> --
> 2.47.2

Re: [PATCH next] wifi: rtw89: mcc: prevent shift wrapping in rtw89_core_mlsr_switch()

Posted by Dan Carpenter 6 months, 3 weeks ago

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

RE: [PATCH next] wifi: rtw89: mcc: prevent shift wrapping in rtw89_core_mlsr_switch()

Posted by Zong-Zhe Yang 6 months, 3 weeks ago

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