rule_locs is allocated in ethtool_get_rxnfc and the size is determined by
rule_cnt from user space. So rule_cnt needs to be check before using
rule_locs to avoid OOB writing or NULL pointer dereference.
Fixes: c5d511c49587 ("net: bcmasp: Add support for wake on net filters")
Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
---
drivers/net/ethernet/broadcom/asp2/bcmasp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
index d63d321f3e7b..4df2ca871af8 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
@@ -535,6 +535,9 @@ void bcmasp_netfilt_get_all_active(struct bcmasp_intf *intf, u32 *rule_locs,
int j = 0, i;
for (i = 0; i < NUM_NET_FILTERS; i++) {
+ if (j == *rule_cnt)
+ break;
+
if (!priv->net_filters[i].claimed ||
priv->net_filters[i].port != intf->port)
continue;
--
2.34.1
On Wed, 2023-09-06 at 17:21 +0800, Hangyu Hua wrote: > rule_locs is allocated in ethtool_get_rxnfc and the size is determined by > rule_cnt from user space. So rule_cnt needs to be check before using > rule_locs to avoid OOB writing or NULL pointer dereference. > > Fixes: c5d511c49587 ("net: bcmasp: Add support for wake on net filters") > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > --- > drivers/net/ethernet/broadcom/asp2/bcmasp.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c > index d63d321f3e7b..4df2ca871af8 100644 > --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c > +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c > @@ -535,6 +535,9 @@ void bcmasp_netfilt_get_all_active(struct bcmasp_intf *intf, u32 *rule_locs, > int j = 0, i; > > for (i = 0; i < NUM_NET_FILTERS; i++) { > + if (j == *rule_cnt) > + break; Side note: it's a bit unfortunate/confusing that the drivers can arbitrary return -EMSGSIZE or silently truncate the list. I think it would be clearer if we could stick to single behavior - and I'll vote for -EMSGSIZE. Cheers, Paolo
On 7/9/2023 17:44, Paolo Abeni wrote: > On Wed, 2023-09-06 at 17:21 +0800, Hangyu Hua wrote: >> rule_locs is allocated in ethtool_get_rxnfc and the size is determined by >> rule_cnt from user space. So rule_cnt needs to be check before using >> rule_locs to avoid OOB writing or NULL pointer dereference. >> >> Fixes: c5d511c49587 ("net: bcmasp: Add support for wake on net filters") >> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >> --- >> drivers/net/ethernet/broadcom/asp2/bcmasp.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c >> index d63d321f3e7b..4df2ca871af8 100644 >> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c >> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c >> @@ -535,6 +535,9 @@ void bcmasp_netfilt_get_all_active(struct bcmasp_intf *intf, u32 *rule_locs, >> int j = 0, i; >> >> for (i = 0; i < NUM_NET_FILTERS; i++) { >> + if (j == *rule_cnt) >> + break; > > Side note: it's a bit unfortunate/confusing that the drivers can > arbitrary return -EMSGSIZE or silently truncate the list. I think it > would be clearer if we could stick to single behavior - and I'll vote > for -EMSGSIZE. I see. I used break directly here beacause this function is defined as void. But since you mentioned this I will fix this out. Thanks, Hangyu > > Cheers, > > Paolo >
On 9/6/23 2:21 AM, Hangyu Hua wrote: > rule_locs is allocated in ethtool_get_rxnfc and the size is determined by > rule_cnt from user space. So rule_cnt needs to be check before using > rule_locs to avoid OOB writing or NULL pointer dereference. > > Fixes: c5d511c49587 ("net: bcmasp: Add support for wake on net filters") > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> Reviewed-by: Justin Chen <justin.chen@broadcom.com> > --- > drivers/net/ethernet/broadcom/asp2/bcmasp.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c > index d63d321f3e7b..4df2ca871af8 100644 > --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c > +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c > @@ -535,6 +535,9 @@ void bcmasp_netfilt_get_all_active(struct bcmasp_intf *intf, u32 *rule_locs, > int j = 0, i; > > for (i = 0; i < NUM_NET_FILTERS; i++) { > + if (j == *rule_cnt) > + break; > + > if (!priv->net_filters[i].claimed || > priv->net_filters[i].port != intf->port) > continue;
© 2016 - 2024 Red Hat, Inc.