| Message ID | 20200510122759.11774-1-gpiccoli@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [X/B] ethtool: Ensure new ring parameters are within bounds during SRINGPARAM | expand |
LGTM. Thanks, Guilherme. Acked-by: Kamal Mostafa <kamal@canonical.com> -Kamal On Sun, May 10, 2020 at 09:27:59AM -0300, Guilherme G. Piccoli wrote: > From: Eugenia Emantayev <eugenia@mellanox.com> > > BugLink: https://bugs.launchpad.net/bugs/1874444 > > Add a sanity check to ensure that all requested ring parameters > are within bounds, which should reduce errors in driver implementation. > > Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com> > Signed-off-by: Tariq Toukan <tariqt@mellanox.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit 37e2d99b59c4765112533a1d38174fea58d28a51) > Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com> > --- > > > Hi folks, this was tested by me in AWS instance, ena driver is affected > by it (in other words, no ring validation in the driver), but many other > drivers could suffer from this, hence I'd recommend of course to merge > in all releases. > > Thanks in advance for reviews, > > Guilherme > > > net/core/ethtool.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 3702a24a8cf5..86b4f474b46e 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -1705,14 +1705,23 @@ static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr) > > static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr) > { > - struct ethtool_ringparam ringparam; > + struct ethtool_ringparam ringparam, max = { .cmd = ETHTOOL_GRINGPARAM }; > > - if (!dev->ethtool_ops->set_ringparam) > + if (!dev->ethtool_ops->set_ringparam || !dev->ethtool_ops->get_ringparam) > return -EOPNOTSUPP; > > if (copy_from_user(&ringparam, useraddr, sizeof(ringparam))) > return -EFAULT; > > + dev->ethtool_ops->get_ringparam(dev, &max); > + > + /* ensure new ring parameters are within the maximums */ > + if (ringparam.rx_pending > max.rx_max_pending || > + ringparam.rx_mini_pending > max.rx_mini_max_pending || > + ringparam.rx_jumbo_pending > max.rx_jumbo_max_pending || > + ringparam.tx_pending > max.tx_max_pending) > + return -EINVAL; > + > return dev->ethtool_ops->set_ringparam(dev, &ringparam); > } > > -- > 2.25.2 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 10.05.20 14:27, Guilherme G. Piccoli wrote: > From: Eugenia Emantayev <eugenia@mellanox.com> > > BugLink: https://bugs.launchpad.net/bugs/1874444 > > Add a sanity check to ensure that all requested ring parameters > are within bounds, which should reduce errors in driver implementation. > > Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com> > Signed-off-by: Tariq Toukan <tariqt@mellanox.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit 37e2d99b59c4765112533a1d38174fea58d28a51) > Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > > > Hi folks, this was tested by me in AWS instance, ena driver is affected > by it (in other words, no ring validation in the driver), but many other > drivers could suffer from this, hence I'd recommend of course to merge > in all releases. > > Thanks in advance for reviews, > > Guilherme > > > net/core/ethtool.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 3702a24a8cf5..86b4f474b46e 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -1705,14 +1705,23 @@ static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr) > > static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr) > { > - struct ethtool_ringparam ringparam; > + struct ethtool_ringparam ringparam, max = { .cmd = ETHTOOL_GRINGPARAM }; > > - if (!dev->ethtool_ops->set_ringparam) > + if (!dev->ethtool_ops->set_ringparam || !dev->ethtool_ops->get_ringparam) > return -EOPNOTSUPP; > > if (copy_from_user(&ringparam, useraddr, sizeof(ringparam))) > return -EFAULT; > > + dev->ethtool_ops->get_ringparam(dev, &max); > + > + /* ensure new ring parameters are within the maximums */ > + if (ringparam.rx_pending > max.rx_max_pending || > + ringparam.rx_mini_pending > max.rx_mini_max_pending || > + ringparam.rx_jumbo_pending > max.rx_jumbo_max_pending || > + ringparam.tx_pending > max.tx_max_pending) > + return -EINVAL; > + > return dev->ethtool_ops->set_ringparam(dev, &ringparam); > } > >
On 2020-05-10 09:27:59 , Guilherme G. Piccoli wrote: > From: Eugenia Emantayev <eugenia@mellanox.com> > > BugLink: https://bugs.launchpad.net/bugs/1874444 > > Add a sanity check to ensure that all requested ring parameters > are within bounds, which should reduce errors in driver implementation. > > Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com> > Signed-off-by: Tariq Toukan <tariqt@mellanox.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit 37e2d99b59c4765112533a1d38174fea58d28a51) > Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com> > --- > > > Hi folks, this was tested by me in AWS instance, ena driver is affected > by it (in other words, no ring validation in the driver), but many other > drivers could suffer from this, hence I'd recommend of course to merge > in all releases. > > Thanks in advance for reviews, > > Guilherme > > > net/core/ethtool.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 3702a24a8cf5..86b4f474b46e 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -1705,14 +1705,23 @@ static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr) > > static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr) > { > - struct ethtool_ringparam ringparam; > + struct ethtool_ringparam ringparam, max = { .cmd = ETHTOOL_GRINGPARAM }; > > - if (!dev->ethtool_ops->set_ringparam) > + if (!dev->ethtool_ops->set_ringparam || !dev->ethtool_ops->get_ringparam) > return -EOPNOTSUPP; > > if (copy_from_user(&ringparam, useraddr, sizeof(ringparam))) > return -EFAULT; > > + dev->ethtool_ops->get_ringparam(dev, &max); > + > + /* ensure new ring parameters are within the maximums */ > + if (ringparam.rx_pending > max.rx_max_pending || > + ringparam.rx_mini_pending > max.rx_mini_max_pending || > + ringparam.rx_jumbo_pending > max.rx_jumbo_max_pending || > + ringparam.tx_pending > max.tx_max_pending) > + return -EINVAL; > + > return dev->ethtool_ops->set_ringparam(dev, &ringparam); > } > > -- > 2.25.2 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 3702a24a8cf5..86b4f474b46e 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1705,14 +1705,23 @@ static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr) static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr) { - struct ethtool_ringparam ringparam; + struct ethtool_ringparam ringparam, max = { .cmd = ETHTOOL_GRINGPARAM }; - if (!dev->ethtool_ops->set_ringparam) + if (!dev->ethtool_ops->set_ringparam || !dev->ethtool_ops->get_ringparam) return -EOPNOTSUPP; if (copy_from_user(&ringparam, useraddr, sizeof(ringparam))) return -EFAULT; + dev->ethtool_ops->get_ringparam(dev, &max); + + /* ensure new ring parameters are within the maximums */ + if (ringparam.rx_pending > max.rx_max_pending || + ringparam.rx_mini_pending > max.rx_mini_max_pending || + ringparam.rx_jumbo_pending > max.rx_jumbo_max_pending || + ringparam.tx_pending > max.tx_max_pending) + return -EINVAL; + return dev->ethtool_ops->set_ringparam(dev, &ringparam); }