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 phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A1147C4332F for ; Sun, 4 Dec 2022 20:36:38 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id E84A5854E2; Sun, 4 Dec 2022 21:36:35 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="Fzta7Atp"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 142EC85428; Sun, 4 Dec 2022 21:36:35 +0100 (CET) Received: from mail-lj1-x230.google.com (mail-lj1-x230.google.com [IPv6:2a00:1450:4864:20::230]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id BC848854E2 for ; Sun, 4 Dec 2022 21:36:32 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=szymon.heidrich@gmail.com Received: by mail-lj1-x230.google.com with SMTP id x6so11388680lji.10 for ; Sun, 04 Dec 2022 12:36:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=HMIn7qCfvenFvVeI/q9LxL8IJT/Y6OIHQ43lAF0TOIw=; b=Fzta7AtpEgjth4KGR1vjmay072SA8SwPBNQUNBNYxIk7soNDa+NGD2INgOFyBK/hzr UdNWZp1j3DIqSM448y6wJwqRdzpAplXTJ8CrnGaUncn4HnKUPg+NcCRM12GKMeUa/HyZ 8DipQ0MdhaPF9ey7q7ID3Gvuvg9Fr5l8elESdqTQAb757WYnBX+3+uuaMTnoTSBb5omt qu/1xeDLsMH+SyCTd2Wl/WOI4IJ7EX7hCaXRASQGS++DQqw3iA9HkYPobO3G3OXXWFEF nOnHXQUHEhQxTr4pel0Cxj4bz1bc/ANSkH1szMW2qaJT+34V+k+JXisJjjHe18HGObYJ 8jpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=HMIn7qCfvenFvVeI/q9LxL8IJT/Y6OIHQ43lAF0TOIw=; b=t5pY4OxJcbRfbItvWoXoUVqZLic//WHdFMTp2B7D1nqt0GqA6zIs4o2zMheyg3FROc jfwSjUePmkYu8zE3Z43uFUW5zwJKgv8rAq4ncSNylC3+VlIMVwNip2tmEtjxZwMLmTXD yQ+gVAnvmpiYKreInnrvuKZJ4cftKqu2opFUaqRHjp687QIU7jvgjvysmgyJo9JCqmzb VaBWamjBrDpmRLu0R9lNCt0mqCGv/yID6Oopt+qYmDKDrktoBGxCAj8yEtP4H1xR99k1 gdA2eGRF+uqYvEyD+KxgwRL2/wIZBcHbXGYFToVQ2QBcUfcr7TQajoiDWiZyVjtJpVoc YxlQ== X-Gm-Message-State: ANoB5pl2Fl/pCulrOETkAkqfWpv9xaJCWM8Ejy9r1ADUjKTcSwnk6HpH 4WvObQzr9WDVoAy3talMqWw= X-Google-Smtp-Source: AA0mqf7zAm6zb6bGQvZL3SzIBycIEi8KShNne+iZsfoRbJXRfLj9mh90kju0bZrC/mlLBoH7xBkvcg== X-Received: by 2002:a05:651c:12c1:b0:277:2fd5:482 with SMTP id 1-20020a05651c12c100b002772fd50482mr21271488lje.194.1670186191991; Sun, 04 Dec 2022 12:36:31 -0800 (PST) Received: from [192.168.1.20] (159-205-155-92.adsl.inetia.pl. [159.205.155.92]) by smtp.gmail.com with ESMTPSA id p10-20020ac24eca000000b004b56d00b2d1sm391314lfr.285.2022.12.04.12.36.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 04 Dec 2022 12:36:31 -0800 (PST) Message-ID: <303069fd-dccb-b11d-e264-ad8cba7d5587@gmail.com> Date: Sun, 4 Dec 2022 21:36:27 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Subject: Re: [PATCH] Enforce buffer boundaries on RNDIS USB Gadget Content-Language: en-US To: Marek Vasut , Fabio Estevam , Lukasz Majewski Cc: u-boot@lists.denx.de References: <20221117194438.67015-1-szymon.heidrich@gmail.com> <0625e391-ce84-20be-e89e-94f4e961c44c@gmail.com> From: Szymon Heidrich In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On 04/12/2022 20:12, Marek Vasut wrote: > On 12/3/22 15:59, Szymon Heidrich wrote: >> On 20/11/2022 16:02, Fabio Estevam wrote: >>> Szymon, >>> >>> On Thu, Nov 17, 2022 at 4:46 PM Szymon Heidrich >>> wrote: >>>> >>>> Prevent access to arbitrary memory locations in gen_ndis_set_resp >>>> via manipulation of buf->InformationBufferOffset. Lack of validation >>>> of BufOffset could be exploited to dump arbitrary memory contents >>>> via NDIS packet filter. >>>> >>>> Signed-off-by: Szymon Heidrich >>> >>> Please run ./scripts/get_maintainer.pl on your patch and copy the maintainers. >>> >> >> Hello Fabio, >> >> Sorry I missed adding Lukasz and Marek - I'll keep that in mind for future. >> >> Is there anything else missing from my side? > > There have been various security fixes recently which broke other things, so I am being careful now. > Sure, I completely understand that. Thank you for your time and review. >>>> diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c >>>> index 13c327ea38..3948f2cc9a 100644 >>>> --- a/drivers/usb/gadget/rndis.c >>>> +++ b/drivers/usb/gadget/rndis.c >>>> @@ -855,14 +855,17 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf) >>>>          rndis_set_cmplt_type    *resp; >>>>          rndis_resp_t            *r; >>>> >>>> +       BufLength = get_unaligned_le32(&buf->InformationBufferLength); >>>> +       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset); >>>> +       if ((BufOffset > RNDIS_MAX_TOTAL_SIZE - 8) || >>>> +           (BufLength > RNDIS_MAX_TOTAL_SIZE - 8 - BufOffset)) >>>> +               return -EINVAL; >>>> + >>>>          r = rndis_add_response(configNr, sizeof(rndis_set_cmplt_type)); >>>>          if (!r) >>>>                  return -ENOMEM; >>>>          resp = (rndis_set_cmplt_type *) r->buf; >>>> >>>> -       BufLength = get_unaligned_le32(&buf->InformationBufferLength); >>>> -       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset); >>>> - > > Reading through the RNDIS code, do you think the rndis_query_response and others which use buffer/offset data from the message should also be sanitized the same way ? I can imagine the query can be used to do test for 1bit of data all over the memory too. I added the extra validation in rndis_set_response as with the current implementation it is possible to manipulate InformationBufferOffset to exploit OID_GEN_CURRENT_PACKET_FILTER to set arbitrary memory contents within a 32byte offset as the devices packet filter. This value may be next retrieved using gen_ndis_query_resp so one may extract specific memory regions two bytes a time. As for rndis_query_response I didn't touch it as the buffer offset and length passed to gen_ndis_query_resp are not used. Please let me know in case I'm missing something.