From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.5 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AC9F4C7618F for ; Wed, 17 Jul 2019 18:42:06 +0000 (UTC) Received: from dpdk.org (dpdk.org [92.243.14.124]) by mail.kernel.org (Postfix) with ESMTP id 12BA32184B for ; Wed, 17 Jul 2019 18:42:05 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=networkplumber-org.20150623.gappssmtp.com header.i=@networkplumber-org.20150623.gappssmtp.com header.b="SZYnXaEi" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 12BA32184B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=networkplumber.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=dev-bounces@dpdk.org Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4CA501BDEA; Wed, 17 Jul 2019 20:42:05 +0200 (CEST) Received: from mail-pg1-f193.google.com (mail-pg1-f193.google.com [209.85.215.193]) by dpdk.org (Postfix) with ESMTP id B97E31B9DE for ; Wed, 17 Jul 2019 20:42:04 +0200 (CEST) Received: by mail-pg1-f193.google.com with SMTP id i18so11580010pgl.11 for ; Wed, 17 Jul 2019 11:42:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=rtryZYk8zFD5adNCYL413SjvkJHyEhSwi5yJ8qTZDNk=; b=SZYnXaEiQqr7ETilvb6L9ZK8UVEon6LtpoSxB8TM1XTu+43V3N4Bp9sITQou0CbU6M eRsGEkKkJog8bz7+VLSAvQJOOSIoa5rfLYb9ijlzGYm3Qg9YZooL4+45mjRyL1XiB+db CoeV3UBcenLBUDTD6rKoVpHqz1NQSYvSk7kqh6IaCzTAAI0Pxck0ZMIe1x7EJ9mrE+Wa UAJ9DSWDk9VMiTfZ6R3TvaBUO6WdcS/buqGHZafogHrgMMCtKjFOhKYynDrjmTdoGGWq LPxT6s023gdxrwWNzF2//NRYtbnJ4Y96KHmb1J6y1XNfq20UAL75Pqozr5HqwU4vdzOj M4Sw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=rtryZYk8zFD5adNCYL413SjvkJHyEhSwi5yJ8qTZDNk=; b=IzWXBlKp3STpzi8jlO/GEGSt0AlWHPetcQ/LOeWR3cvHTsquczAG9KA7qRXnl0B39E J28pDVu1gT2/Vq+2N3d7dB1dI3dWB21aoQP3WSY1kQN4+vqaDo0T8chHNHgPmvGj3Yis PesYP51ViF+sAnZbQlMiRE7071oBKU3+gijns7OnAMCNNNaGnuvAelXURaMy0gf26IrG EVzZ6uxSH5gScHoWmgNtmrVG70BNfVLdhNBoku3XStHQb1YbJNhcub73zrSivwd0WJ/8 gvhaSQMmwrDg5hkZxGmZ0Q4jR/zpko0mv6YAoGtq8W1eO10YBna+VdquS98QZRnbQKyo nbFQ== X-Gm-Message-State: APjAAAVWjkQYbKfdl9fUIfO2MLm5m1ezBmhq7FcGqs9nzcO5nWNlEFfr 0JDGipYMu3Go/xdvlgAmb6chv/os X-Google-Smtp-Source: APXvYqz2XGvDJo2OVy1fxx6o59bwSS0hqBffUUn9T30X6rBJVO9zsjaFnGDPwvNJGPwhnzDE/VQzxg== X-Received: by 2002:a63:dd16:: with SMTP id t22mr11150325pgg.140.1563388923617; Wed, 17 Jul 2019 11:42:03 -0700 (PDT) Received: from xps13 ([67.23.203.6]) by smtp.gmail.com with ESMTPSA id br18sm20819374pjb.20.2019.07.17.11.42.02 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 17 Jul 2019 11:42:03 -0700 (PDT) Date: Wed, 17 Jul 2019 11:42:00 -0700 From: Stephen Hemminger To: Aaron Conole Cc: dev@dpdk.org, Olivier Matz , Andrew Rybchenko , Ferruh Yigit , Michael Santana Message-ID: <20190717114200.0f2e79d4@xps13> In-Reply-To: References: <20190710183342.6459-1-aconole@redhat.com> <20190710114230.7c171c7a@hermes.lan> <20190710122756.62ec19a9@hermes.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] rte_ether: force format string for unformat_addr X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, 10 Jul 2019 16:31:59 -0400 Aaron Conole wrote: > Stephen Hemminger writes: > > > On Wed, 10 Jul 2019 15:13:02 -0400 > > Aaron Conole wrote: > > > >> Stephen Hemminger writes: > >> > >> > On Wed, 10 Jul 2019 14:33:42 -0400 > >> > Aaron Conole wrote: > >> > > >> >> rte_ether_unformation_addr is very lax in what it accepts now, including > >> >> ethernet addresses formatted ambiguously as "x:xx:x:xx:x:xx". However, > >> >> previously this behavior was enforced via the my_ether_aton which would > >> >> fail ambiguously formatted values. > >> >> > >> >> Reported-by: Michael Santana > >> >> Fixes: 596d31092d32 ("net: add function to convert string to ethernet address") > >> >> Signed-off-by: Aaron Conole > >> >> --- > >> >> lib/librte_net/rte_ether.c | 6 ++++-- > >> >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/lib/librte_net/rte_ether.c b/lib/librte_net/rte_ether.c > >> >> index 8d040173c..4f252b813 100644 > >> >> --- a/lib/librte_net/rte_ether.c > >> >> +++ b/lib/librte_net/rte_ether.c > >> >> @@ -45,7 +45,8 @@ rte_ether_unformat_addr(const char *s, struct rte_ether_addr *ea) > >> >> if (n == 6) { > >> >> /* Standard format XX:XX:XX:XX:XX:XX */ > >> >> if (o0 > UINT8_MAX || o1 > UINT8_MAX || o2 > UINT8_MAX || > >> >> - o3 > UINT8_MAX || o4 > UINT8_MAX || o5 > UINT8_MAX) { > >> >> + o3 > UINT8_MAX || o4 > UINT8_MAX || o5 > UINT8_MAX || > >> >> + strlen(s) != RTE_ETHER_ADDR_FMT_SIZE - 1) { > >> >> rte_errno = ERANGE; > >> >> return -1; > >> >> } > >> >> @@ -58,7 +59,8 @@ rte_ether_unformat_addr(const char *s, struct rte_ether_addr *ea) > >> >> ea->addr_bytes[5] = o5; > >> >> } else if (n == 3) { > >> >> /* Support the format XXXX:XXXX:XXXX */ > >> >> - if (o0 > UINT16_MAX || o1 > UINT16_MAX || o2 > UINT16_MAX) { > >> >> + if (o0 > UINT16_MAX || o1 > UINT16_MAX || o2 > UINT16_MAX || > >> >> + strlen(s) != RTE_ETHER_ADDR_FMT_SIZE - 4) { > >> >> rte_errno = ERANGE; > >> >> return -1; > >> >> } > >> > > >> > NAK > >> > Skipping leading zero should be ok. There is no need for this patch. > >> > >> Is it intended to skip the leading 0? Why not the trailing 0? I'm not > >> familiar with the format that is used here (example - X:XX:X:XX:X) > >> > >> It isn't described in any RFC I could find (but I only did a small > >> search). Even in IEEE, the format is always a full octet. > >> > >> > The current behavior is superset of what standard ether_aton accepts. > >> > >> Okay, but it introduces a test failure for the cmdline tests and then > >> that test will need a few lines removed for 'unsuccessful' formats. > >> > >> ether_aton is much more rigid in the formats it accepts, so the test > >> case is enforcing that. I guess either the current behavior of this > >> function changes (and since it is a new behavior of the cmdline parser, > >> I would think it should be changed) or the test case should be changed > >> to adopt it. > > > > BSD ether_aton is: > > /* > > * Convert an ASCII representation of an ethernet address to binary form. > > */ > > struct ether_addr * > > ether_aton_r(const char *a, struct ether_addr *e) > > { > > int i; > > unsigned int o0, o1, o2, o3, o4, o5; > > > > i = sscanf(a, "%x:%x:%x:%x:%x:%x", &o0, &o1, &o2, &o3, &o4, &o5); > > if (i != 6) > > return (NULL); > > e->octet[0]=o0; > > e->octet[1]=o1; > > e->octet[2]=o2; > > e->octet[3]=o3; > > e->octet[4]=o4; > > e->octet[5]=o5; > > return (e); > > } > > Your implementation fixes the above by bounds checking each octet > to enforce that in the 6-octet form, each octet is bound to the region > 00-ff. > > The BSD example only accepts a 6-octet form. Your version is intended > to accept both colon forms so x:x:x will successfully parse as well > (interpreted on the XXXX:XXXX:XXXX side) (ie: mac 02:03:04 or 2:3:4 > would be accepted). Further, accidentally passing an ipv6 address to > this routine (something a user of a cmdline interface might do) could be > parsed as valid (example: 2001:db8:2::1) - which would be the wrong > thing. I think it would be strange for length limits to be enforced in > cmdline parser *after* calling this, but that might be an option for > fixing (so patch cmdline_parse_etheraddr to do a length check after the > unformat_addr call). > > I guess I'm not sure what the *best* fix would be. I think the most > sane fix is what I've put in since it will only allow the commonly > accepted notation, and not allow ad-hoc accidents. Higher layers (like > cmdline parsers) are free to implement routines that reformat the lax > forms (like you might want to allow a user to pass) into more > restrictive forms required by a lower layer (like librte_net). I > concede that there could be a more friendly thing to do in some specific > cases - but then we must more strictly validate the *form* (ie: we > have a scanf where one form is a subset of another and will be okay with > some kinds of invalid characters being inserted - allowing for things > like IPV6 addresses looking like ethernet hardware addresses). I have a new version that is closer to original implementation in cmdline_parse_etheraddr. Comparison chart relative to ether_aton Input glibc BSD ORIG NEW 01:23:45:67:89:AB ok ok ok ok 4567:89AB:CDEF BAD BAD ok ok 00:11:22:33:44:55#garbage ok ok BAD BAD 00:11:22:33:44:55 garbage ok ok BAD BAD 0011:2233:4455#garbage BAD BAD BAD BAD 0123:45:67:89:AB BAD BAD BAD BAD 01:23:4567:89:AB BAD BAD BAD BAD 01:23:45:67:89AB BAD BAD BAD BAD 012:345:678:9AB BAD BAD BAD BAD 01:23:45:67:89:ABC ok ok BAD BAD 01:23:45:67:89:A ok ok ok BAD 01:23:45:67:89 BAD BAD BAD BAD 01:23:45:67:89:AB:CD ok ok BAD BAD IN:VA:LI:DC:HA:RS BAD BAD BAD BAD INVA:LIDC:HARS BAD BAD BAD BAD 01 23 45 67 89 AB BAD BAD BAD BAD 01-23-45-67-89-AB BAD BAD BAD BAD 01.23.45.67.89.AB BAD BAD BAD BAD 01,23,45,67,89,AB BAD BAD BAD BAD 01:23:45 BAD BAD ok BAD 01:23:45#:67:89:AB BAD BAD BAD BAD random invalid text BAD BAD BAD BAD random text BAD BAD BAD BAD