Received: by 2002:ac0:a582:0:0:0:0:0 with SMTP id m2-v6csp3884614imm; Mon, 8 Oct 2018 11:05:53 -0700 (PDT) X-Google-Smtp-Source: ACcGV61e5O4DZMLunysA6hFRIozEuK3S8lqvF54oW8L1gwKx02UTiHYmIiwYHvoxFIw4PYRQa+Ko X-Received: by 2002:a62:968a:: with SMTP id s10-v6mr25970609pfk.191.1539021953646; Mon, 08 Oct 2018 11:05:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1539021953; cv=none; d=google.com; s=arc-20160816; b=LFJBy/fTzqfIOH6qZ4kNY6RGVbn4Sdu0slUQThOwvXahLpxSoaz88xdVfDx2Nu9zij YZRHUX7MAEDiQzdhXe7uAJHDWgudp3VeKjiOvckMsyUB3mV+KWUmiovOhIZCNMczreFm WRbJtu6uxMkWNSMuoeyU0+TqT/zBZq9F4xEdmBdYHWK8+omMcJ6gpLmI63sdLITpQNBK ejsG5B+S2LNmFPavkZPlkPpaEcgoD62cjvnNQUYfwXtATq24vN0DgiJjY0DC9gr1/4it Eb6gKjnkudOf3iYhaAaE7ST8aFO0xMs6WYuVgx/+82iv5K67LPSRH5/84wFvwjUFPAjF K7kA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=yn+hwzZ3nJfjnnrqlOGPlZAldwD5mBboiVIrkVHXfKM=; b=mrWTnl8nCV+N3D4WdPe0YH82Cfxn8LxW8Cu8TR6wMJ2dot2hLLOkT1IzGcgZoIVU9W rJbbl1/Ee4cXlhjLSz6sf+fhRD6Db/uG3pC5tGrDjrc9h6KpUNUI0eDAEzO/e7/0kJJ/ Z6c88F4kndSP6PIX2cW2JvuGGynJ/1AhRKat9J+3NQDdaJLj8kjBo8+ZWnl2WQ5bShNi Y698G4qtN8ZxTrlKxfNI7PySwylv1IzBUAK5wchLD9xtfqeCNXf6CSMjHD17NLLlWoWi 1Jz5kKFT6evBvROC93ZsaZ7S7OOqwQRie1ewoWSJxDYE68A9df/lqiqTeG8TP9+ZX4lH AH1A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id x17-v6si17523322pgl.414.2018.10.08.11.05.38; Mon, 08 Oct 2018 11:05:53 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726786AbeJIBQq (ORCPT + 99 others); Mon, 8 Oct 2018 21:16:46 -0400 Received: from mx2.suse.de ([195.135.220.15]:40834 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726391AbeJIBQp (ORCPT ); Mon, 8 Oct 2018 21:16:45 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 1A33DAE60; Mon, 8 Oct 2018 18:03:49 +0000 (UTC) Received: by unicorn.suse.cz (Postfix, from userid 1000) id 1264BA0A65; Mon, 8 Oct 2018 20:03:47 +0200 (CEST) Date: Mon, 8 Oct 2018 20:03:47 +0200 From: Michal Kubecek To: Wenwen Wang Cc: Kangjie Lu , "David S. Miller" , Florian Fainelli , Kees Cook , Andrew Lunn , Edward Cree , Ilya Lesokhin , Yury Norov , Alan Brady , Stephen Hemminger , "open list:NETWORKING [GENERAL]" , open list Subject: Re: [PATCH] ethtool: fix a privilege escalation bug Message-ID: <20181008180346.GA9504@unicorn.suse.cz> References: <1539013777-1625-1-git-send-email-wang6495@umn.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1539013777-1625-1-git-send-email-wang6495@umn.edu> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 08, 2018 at 10:49:35AM -0500, Wenwen Wang wrote: > In dev_ethtool(), the eth command 'ethcmd' is firstly copied from the > use-space buffer 'useraddr' and checked to see whether it is > ETHTOOL_PERQUEUE. If yes, the sub-command 'sub_cmd' is further copied from > the user space. Otherwise, 'sub_cmd' is the same as 'ethcmd'. Next, > according to 'sub_cmd', a permission check is enforced through the function > ns_capable(). For example, the permission check is required if 'sub_cmd' is > ETHTOOL_SCOALESCE, but it is not necessary if 'sub_cmd' is > ETHTOOL_GCOALESCE, as suggested in the comment "Allow some commands to be > done by anyone". The following execution invokes different handlers > according to 'ethcmd'. Specifically, if 'ethcmd' is ETHTOOL_PERQUEUE, > ethtool_set_per_queue() is called. In ethtool_set_per_queue(), the kernel > object 'per_queue_opt' is copied again from the user-space buffer > 'useraddr' and 'per_queue_opt.sub_command' is used to determine which > operation should be performed. Given that the buffer 'useraddr' is in the > user space, a malicious user can race to change the sub-command between the > two copies. In particular, the attacker can supply ETHTOOL_PERQUEUE and > ETHTOOL_GCOALESCE to bypass the permission check in dev_ethtool(). Then > before ethtool_set_per_queue() is called, the attacker changes > ETHTOOL_GCOALESCE to ETHTOOL_SCOALESCE. In this way, the attacker can > bypass the permission check and execute ETHTOOL_SCOALESCE. > > This patch enforces a check in ethtool_set_per_queue() after the second > copy from 'useraddr'. If the sub-command is different from the one obtained > in the first copy in dev_ethtool(), an error code EINVAL will be returned. > > Signed-off-by: Wenwen Wang Reviewed-by: Michal Kubecek I'm just not sure if "privilege escalation" is an appropriate term but at least some sources define it loosely enough to cover also a simple permission check bypass like this. Michal Kubecek > --- > net/core/ethtool.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index c9993c6..ccb337e 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -2462,13 +2462,17 @@ static int ethtool_set_per_queue_coalesce(struct net_device *dev, > return ret; > } > > -static int ethtool_set_per_queue(struct net_device *dev, void __user *useraddr) > +static int ethtool_set_per_queue(struct net_device *dev, > + void __user *useraddr, u32 sub_cmd) > { > struct ethtool_per_queue_op per_queue_opt; > > if (copy_from_user(&per_queue_opt, useraddr, sizeof(per_queue_opt))) > return -EFAULT; > > + if (per_queue_opt.sub_command != sub_cmd) > + return -EINVAL; > + > switch (per_queue_opt.sub_command) { > case ETHTOOL_GCOALESCE: > return ethtool_get_per_queue_coalesce(dev, useraddr, &per_queue_opt); > @@ -2838,7 +2842,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) > rc = ethtool_get_phy_stats(dev, useraddr); > break; > case ETHTOOL_PERQUEUE: > - rc = ethtool_set_per_queue(dev, useraddr); > + rc = ethtool_set_per_queue(dev, useraddr, sub_cmd); > break; > case ETHTOOL_GLINKSETTINGS: > rc = ethtool_get_link_ksettings(dev, useraddr); > -- > 2.7.4 >