From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tariq Toukan Subject: Re: [PATCH net-next 1/3] ethtool: Ensure new ring parameters are within bounds during SRINGPARAM Date: Tue, 9 Jan 2018 09:30:05 +0200 Message-ID: References: <1515420026-11970-1-git-send-email-tariqt@mellanox.com> <1515420026-11970-2-git-send-email-tariqt@mellanox.com> <20180108182342.084c5d60@cakuba.netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "David S. Miller" , netdev@vger.kernel.org, Eran Ben Elisha , Eugenia Emantayev To: Jakub Kicinski , Tariq Toukan Return-path: Received: from mail-he1eur01on0077.outbound.protection.outlook.com ([104.47.0.77]:8928 "EHLO EUR01-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932236AbeAIHaQ (ORCPT ); Tue, 9 Jan 2018 02:30:16 -0500 In-Reply-To: <20180108182342.084c5d60@cakuba.netronome.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 09/01/2018 4:23 AM, Jakub Kicinski wrote: > On Mon, 8 Jan 2018 16:00:24 +0200, Tariq Toukan wrote: >> From: Eugenia Emantayev >> >> Add a sanity check to ensure that all requested ring parameters >> are within bounds, which should reduce errors in driver implementation. > > (y) > >> Signed-off-by: Eugenia Emantayev >> Signed-off-by: Tariq Toukan >> --- >> 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 50a79203043b..9ea7cd52fde0 100644 >> --- a/net/core/ethtool.c >> +++ b/net/core/ethtool.c >> @@ -1704,14 +1704,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); > > Perhaps check the return value here? It's pretty unlikely but > get_ringparam may fail. > get_ringparam NDO returns void. >> + /* 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); >> } >>