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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 856D1EEAA7B for ; Thu, 14 Sep 2023 23:38:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230415AbjINXiX (ORCPT ); Thu, 14 Sep 2023 19:38:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60264 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230412AbjINXiV (ORCPT ); Thu, 14 Sep 2023 19:38:21 -0400 Received: from mail-pf1-x436.google.com (mail-pf1-x436.google.com [IPv6:2607:f8b0:4864:20::436]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BE8BD2709 for ; Thu, 14 Sep 2023 16:38:17 -0700 (PDT) Received: by mail-pf1-x436.google.com with SMTP id d2e1a72fcca58-68fbb10dec7so1422164b3a.3 for ; Thu, 14 Sep 2023 16:38:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1694734697; x=1695339497; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=QVyuGB/xvHjbsQ6Zy6WC5UERFVDAMx/lplVOWzlB7nk=; b=Bb1gd0oCn0kV4ULdPhXKK99PK8gVenpWGW9KDzmBileH9K41rHs1rZESe632EcuKPU EVjE2Jzv8r6n2pnoD6m6VHvQy9X1HjocaHkie1wJ6Fe+s9BiY1iwqhnvTNpZH0A8UAvZ +ftpp737dtCN7uk+jTlKhgI3zyvujzln3697Q= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694734697; x=1695339497; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=QVyuGB/xvHjbsQ6Zy6WC5UERFVDAMx/lplVOWzlB7nk=; b=m0qThQVcGvGSiwkfZSzmjuVw1kXbJsVTyFZ99l7tJlVP1WdQ7uxw4a9zXgRJn0UNFB GmATYY9sl+AM3eI+2qZS0NJ5Jf031FlL8rc0OAJ/TeSi/Buy4XHvXYgHXtlTRMpYPSL4 0v8thdVVvf4EzY2GIl2rb4oa1VhCvxo0r0sKPUsTATO7IHitkqvJxa9sdyvSERz6tlU9 lrOsDgAEqA+lgnfti44tLGfD6gA18/4hZ72KTcKPT75dVsrBjFDFuFKgJNROkYlRXMII 8Zp4FJU1wCdTUnqLCxPvRgpscKvhQKixPGPbyhY9oHdmrn/FVuO62/fFrurCN1LeDy1f NLvw== X-Gm-Message-State: AOJu0YwUsBExZwgkkhfVBLHAEuPIaKBhZzN00QZmAh9s+6F/ix+nyCKr 478EdAn9mZ66oSC/nqoBItiqMaEpqpEKs+7x6Gg= X-Google-Smtp-Source: AGHT+IEavKprO9EKeotlIDh1wQtMwXnrWhscNhhuioVis4w+5F1efm6qbgrdiJJhGjthFuyMn0czcg== X-Received: by 2002:a05:6a00:22d1:b0:68e:3772:4e40 with SMTP id f17-20020a056a0022d100b0068e37724e40mr135414pfj.3.1694734697224; Thu, 14 Sep 2023 16:38:17 -0700 (PDT) Received: from localhost ([2620:15c:9d:2:79c3:9a77:bfee:9881]) by smtp.gmail.com with UTF8SMTPSA id n21-20020aa79055000000b0068a54866ca8sm1796533pfo.134.2023.09.14.16.38.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 14 Sep 2023 16:38:16 -0700 (PDT) Date: Thu, 14 Sep 2023 16:38:13 -0700 From: Brian Norris To: Pin-yen Lin Cc: linux-wireless@vger.kernel.org, Kalle Valo , Polaris Pi , Matthew Wang , linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] wifi: mwifiex: Fix oob check condition in mwifiex_process_rx_packet Message-ID: References: <20230908104308.1546501-1-treapking@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org On Thu, Sep 14, 2023 at 03:09:47PM +0800, Pin-yen Lin wrote: > On Thu, Sep 14, 2023 at 4:31 AM Brian Norris wrote: > > I'd appreciate another review/test from one of the others here > > (Matthew?), even though I know y'all are already working together. I'd still appreciate some comment here. > > > - if ((!memcmp(&rx_pkt_hdr->rfc1042_hdr, bridge_tunnel_header, > > > - sizeof(bridge_tunnel_header))) || > > > - (!memcmp(&rx_pkt_hdr->rfc1042_hdr, rfc1042_header, > > > - sizeof(rfc1042_header)) && > > > - ntohs(rx_pkt_hdr->rfc1042_hdr.snap_type) != ETH_P_AARP && > > > - ntohs(rx_pkt_hdr->rfc1042_hdr.snap_type) != ETH_P_IPX)) { > > > + if (sizeof(*rx_pkt_hdr) + rx_pkt_off <= skb->len && > > > > Are you sure you want this length check to fall back to the non-802.3 > > codepath? Isn't it an error to look like an 802.3 frame but to be too > > small? I'd think we want to drop such packets, not process them as-is. > > I did that because I saw other drivers (e.g., [1], [2]) use similar > approaches, and I assumed that the rest of the pipeline will > eventually drop it if the packet cannot be recognized. But, yes, we > can just drop the packet here if it doesn't look good. > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/intersil/hostap/hostap_80211_rx.c#L1035 > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/intel/ipw2x00/libipw_rx.c#L735 Hmm, I suppose. I'm frankly not sure how exactly all upper layers handle this, but at least in a non-raw mode, we'll drop them. (We might be delivering awfully weird packets to tcpdump though, but this is already a weird situation, if it's such a weird-looking packet.) > > If I'm correct, then this check should move inside the 'if' branch of > > this if/else. > > We can't simply move the check inside the if branch because the > condition also checks rx_pkt_hdr->rfc1042_hdr.snap_type. Though, of > course, it is doable by adding another `if` conditions. Right. I guess this is probably OK as-is: Acked-by: Brian Norris