From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=PWDRmvj4; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id B30E35A0262 for ; Thu, 16 Jul 2026 09:22:43 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1784186562; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eVUdBfCC0hGXcNA6Bv/wgJfWxRadugA5bLw3wwU++I8=; b=PWDRmvj4cummzFqaRmieVl6FH0KN825xVds7bVYAWEMZHqv9yipo9LgysCS78rJS8+weJM MAs8ipyaHnfKualwVM4cP+Yf3qI6XvtTS2knBK6cOJRR3iCZzkgCRVAZLHFFKIqIXMtEdO /TAEyQgs0FZirn/eDA/KjmyoDxW8yvQ= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-543-0bUCv0flNUyhWrU3trWWXA-1; Thu, 16 Jul 2026 03:22:40 -0400 X-MC-Unique: 0bUCv0flNUyhWrU3trWWXA-1 X-Mimecast-MFC-AGG-ID: 0bUCv0flNUyhWrU3trWWXA_1784186559 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-493d742c668so49825395e9.2 for ; Thu, 16 Jul 2026 00:22:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784186559; x=1784791359; h=date:content-transfer-encoding:content-type:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to:content-type; bh=eVUdBfCC0hGXcNA6Bv/wgJfWxRadugA5bLw3wwU++I8=; b=l8tHphI8vOSN2MBisbqdTB2v2fTw5uqXNO+bZlfecpktRk9IBjZpdW2ih2tnjwvn2Q O1Wqx3cKoTkKACA9I5ttiHqznG2Ro+dtS7IREVxq00x1xehJOvVxJqfYDmue0Q5eyHfz uADo/hVP42u0Lbk+BgIle7OYGegyhyInxXkRkGZeg1Lcx+Z4oUW5MWH/eiwJHiDJciYm K9oL4GFv6+4ooY+UytUYRoydPi9gXJBwdH1vrly0iDfrKxBRE8KupFGHoyEBworYXi97 kC/o1+cJ7S3W2Ugtao5+pTwbIPEf4ZHwzuGSEM4gQzsecLU9J54VhqLSfaFM7KNUiC83 jnXQ== X-Gm-Message-State: AOJu0YyLVKzWQwNy2A4rUNb2lE/XVChy1eoUr6R/kCwarix7hMST/8K8 GNLWU2Sujo/W/LFhsWds7o4/xP0eqFGbQrmf0Ew0HBbW07HwWgDQW4Zb3Q17JZ6oTTSPUkOW4dR YatFIX3nKzPHAZvM2jr1NNGqArFsVwk2In2mvd7w8POXNogbBOEWGVmVMmGVdTA== X-Gm-Gg: AfdE7clPCkqI1CMaqK6gg9VFKm3vv65FgrLQtTxyBtjOm404633EX6+yDfay+yNLSOv u/qUCPPKdRd0cFkusx/18MdqybXpdSluCqWq0s7sBTv1s+OJDcsNee2SNW2pU5iXZnGQadhyvWR Onaw0maabZeTPGQqzOOgn+FE+3vmaV27HA1gvILmEiGaEokzBxo/Z7jUfoTCZ10t5y2GkBPzv5A H5uaqoBWSQizs4EXr9jxyK4p0jknedwZnndwTZEL1xk77VHn43p1xUO38lzcHoFi23BACifb5zL /uOsGANJUr5+b6BNF0Vdn8ef59qLLytvZFS8kL3KBDjWXwPV66wibJWqgdn6FN30BifiDxCLaOC V0JQtRKZnKQI= X-Received: by 2002:a05:600c:4e49:b0:490:e5c1:b8b9 with SMTP id 5b1f17b1804b1-4953905c584mr106178095e9.0.1784186559179; Thu, 16 Jul 2026 00:22:39 -0700 (PDT) X-Received: by 2002:a05:600c:4e49:b0:490:e5c1:b8b9 with SMTP id 5b1f17b1804b1-4953905c584mr106177575e9.0.1784186558566; Thu, 16 Jul 2026 00:22:38 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-49541e9425asm62694785e9.11.2026.07.16.00.22.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Jul 2026 00:22:37 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 2/4] dhcp: Make option parsing more robust, explicitly handle options 0 and 255 Message-ID: <20260716092235.0e0e8af3@elisabeth> In-Reply-To: References: <20260715232523.3372714-1-sbrivio@redhat.com> <20260715232523.3372714-3-sbrivio@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Thu, 16 Jul 2026 09:22:37 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: DoLmeSPhoCcK369fzBVomM8qpIw7n-_15JFKzMcMoTU_1784186559 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: QBKMGHFI5HVIFQF32ZDWXAWX3DBNTQNA X-Message-ID-Hash: QBKMGHFI5HVIFQF32ZDWXAWX3DBNTQNA X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, 16 Jul 2026 15:37:13 +1000 David Gibson wrote: > On Thu, Jul 16, 2026 at 01:25:21AM +0200, Stefano Brivio wrote: > > The initial option-scanning loop in dhcp(), so far, ignored options 0 > > (Pad Option, RFC 2132, Section 3.1) and 255 (End Option, RFC 2132, > > Section 3.2). > > > > As a result: > > > > - if we ever encountered option 0 in the middle of option fields > > (never seen in practice), we would potentially terminate the loop > > too early, before scanning remaining options > > > > - a malformed message with an option 255 followed by a length byte > > would (reliably) cause us to terminate as we would exceed the > > allocated size for the 'opts' array, which is detected as buffer > > overflow by the FORTIFY_SOURCE mechanism > > > > The latter was reported as potential vulnerability by AISLE, but it's > > not actually a vulnerability as we always terminate without carrying > > on further handling, and in our security model the guest is able to > > sabotage its own connectivity in any case (for example, a malformed > > frame from the hypervisor would cause us to reset the connection, or > > entirely flooding the flow table would cause inbound connectivity to > > stop working, etc.). > > > > The reported behaviour, however, is indeed a defect, as it affects > > the functional robustness to a hypothetical issue in a DHCP client, > > and that's something we definitely want to fix. > > > > Make the option parsing loop more robust by: > > > > - resizing 'opts' from 255 to 256 elements: there's no particular > > reason to try to save a tiny bit of memory (which shouldn't even > > be allocated in practice) instead of being defensive about it > > > > - explicitly handle options 0 (skip one byte, continue) and 255 (stop > > processing options) in the option-scanning loop > > > > This bug was found and an initial version of the patch was written by > > the AISLE AI security scanning tool (https://aisle.com/platform). > > > > Reported-by: AISLE > > Signed-off-by: Stefano Brivio > > --- > > dhcp.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/dhcp.c b/dhcp.c > > index 1ff8cba..632019a 100644 > > --- a/dhcp.c > > +++ b/dhcp.c > > @@ -49,7 +49,7 @@ struct opt { > > uint8_t c[255]; > > }; > > > > -static struct opt opts[255]; > > +static struct opt opts[256]; > > > > #define DHCPDISCOVER 1 > > #define DHCPOFFER 2 > > @@ -374,6 +374,14 @@ int dhcp(const struct ctx *c, struct iov_tail *data) > > if (!type || !olen) > > return -1; > > > > + if (*type == 255) > > + break; > > + > > + if (*type == 0) { /* Pad Option (RFC 2132, 3.1): one byte */ > > + opt_len--; > > + continue; > > + } > > + > > I don't think this is quite right: at this point we've already > stripped off the non-existent length-byte with IOV_REMOVE_HEADER, > meaning opt_len will get out of sync with iov_tail_size(data). Oops, right, I just tested that we would decrease opt_len "enough" for the loop to terminate when we meet pad options at the end (which, in practice, is the only place where we'll find them nowadays), but indeed it's wrong. > I think we instead need to check for the 1-byte option cases between > the two IOV_REMOVE_HEADER() calls. Right, done. > And.. since presumably the options could theoretically end with some > padding options, That's actually the only place where I've seen the pad option occurring, in a long while. > we probably want the loop to be while (opt_len >= 1) instead of 2. Not strictly needed I think as we don't necessarily want to validate the last bytes. But anyway: > In fact the way we > mix recalculating opt_len from iov_tail_size() with sometimes directly > updating it is pretty nasty. We might be better off with > while ((opt_len = iov_tail_size(data))) ...that looks much more natural indeed. I'm using that in v2. -- Stefano