| Message ID | 20230906092107.19063-2-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix possible OOB write when using rule_buf | expand |
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;
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 >
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;
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(+)