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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AC827CD5BDF for ; Thu, 5 Sep 2024 16:44:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Cc:List-Subscribe: List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Type:MIME-Version:References:Message-ID:Subject:To:Date:From:Reply-To :Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=q9/Cl4pns2tvY2+ex2I7VzPKJmEi9tuK1Vum+Ld5DXw=; b=bUpBi+/tMuK+sUNrBFB+ggJgWr a/DYQ8YllThlB/BpqwzrvNqAhxidsv4QuHljcHtWKffwkQMhsByiQXG8IIjKh5JJiSDl0/ZEVvyk4 dX5b/6kJ41ty22YxfpDU1by+dCQ1JvXAdbekLyGJxCCfkClu12cBcSmHFVueKHBYBGSzw5YsWKW92 UWWpNElvpZLFu9YI5adkyxOIwW3BMTTIDXPFzHYHh9mqIeDuvmZNuxfszFPD68zXnOYASH970C36n CQ/K9StP4pK/PQ4rtvbkdKNI1I5GRz5+1GGLl/U+/IfFpcHS/c8V1kqeHcZh+KGGm8IufRvAX+J6q oeb9ZLjw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1smFb0-00000009Axp-0RYn; Thu, 05 Sep 2024 16:44:34 +0000 Received: from mail-ej1-x62e.google.com ([2a00:1450:4864:20::62e]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1smFZz-00000009AgE-3XUx for linux-arm-kernel@lists.infradead.org; Thu, 05 Sep 2024 16:43:33 +0000 Received: by mail-ej1-x62e.google.com with SMTP id a640c23a62f3a-a83562f9be9so109673366b.0 for ; Thu, 05 Sep 2024 09:43:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1725554609; x=1726159409; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:from:to:cc:subject:date:message-id:reply-to; bh=q9/Cl4pns2tvY2+ex2I7VzPKJmEi9tuK1Vum+Ld5DXw=; b=RDK6ieowmrpHHHbF2LHOeT/6E1qxnsrmHU8Os9sK55W8+uHDN6G3N7ZUkH2441YYHM +SCdg49QAejY1kqIlZHuZwtyd5I4WqxzdX35St/U9YngZup+vXBN54lmKx6rfuzXBmkS bHT4yWufSUrFrtySKLskB3HhX4ksKdQA80cjiyo2oPKxbyltQfRkopbNvUUkZxA8jMHC e7hIYqthuvCktXka/ZGuBLgLZL2/VJsnsFf8ksHjwuoVEwSzr/sVYIP/BCC0mtnnrHsB zDBcgGk7v55XB8kDYDRDCblEGiN+aWe5u3LU9TUCvUPZg/vDVgc2MhtCUOgEofjm0BMj 432Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725554609; x=1726159409; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=q9/Cl4pns2tvY2+ex2I7VzPKJmEi9tuK1Vum+Ld5DXw=; b=CtETDIBXcmysistYZlQixcSjF5yQlFlUMcf58kufKipc34mwHqLO6tiMpglYWqUR1j Hme97UHAwQz44sDHt/YfcH9wZLBDDXBX/yqFgDlZLrAgUlRLA2sMLWurRd1dZzksBtkX g6KKoUVeJMX/RnX3SWCQ6id7wuCsrSxoQxS2olpOkxIhYYnLcxKoYmUjH4H1TKdFoZZX hfZi4jfMZTzjppWw08w1LAdksqsL4llyLmPkyXSOpY+O5vxoVZZ5NGQXPeXTAiywuBeV HKAZAT/JnkZ5Jbb68rm2GquGrb6vJ3wuMYRJYSajzaEpiyyRZFUMCRdTkChqHpoksndN cPmQ== X-Forwarded-Encrypted: i=1; AJvYcCWSCmkfBt74GDG+h87UU/5B9pZj5ymHfFpAuYUr0satpUMh+zZA18pYzNRrQherMsZKyjfGo8u4JV53Df5qKNm/@lists.infradead.org X-Gm-Message-State: AOJu0YxyBpbuioC5D2wiATL8W0kV94xK3f+N89r8LGRAOi4X+o1slEed RkKZFidUwMoHWOK9nmdq0NhI83tVXsy0UyxG0C/aocDDvRPlRZsYmsDBqUsLTTY= X-Google-Smtp-Source: AGHT+IEz20rh2z3FuTn3FqdARHdBZA33h+CRI0+H4sggZcuGroEd9OHEiz6QTkjQzE4FTjN3pv3RUA== X-Received: by 2002:a17:907:dab:b0:a86:743a:a716 with SMTP id a640c23a62f3a-a89b96f8af8mr1230712666b.53.1725554607887; Thu, 05 Sep 2024 09:43:27 -0700 (PDT) Received: from localhost (host-80-182-198-72.retail.telecomitalia.it. [80.182.198.72]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a8a6236d0easm156239566b.102.2024.09.05.09.43.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Sep 2024 09:43:27 -0700 (PDT) From: Andrea della Porta X-Google-Original-From: Andrea della Porta Date: Thu, 5 Sep 2024 18:43:35 +0200 To: Bjorn Helgaas Subject: Re: [PATCH 03/11] PCI: of_property: Sanitize 32 bit PCI address parsed from DT Message-ID: References: <20240903222644.GA126427@bhelgaas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240903222644.GA126427@bhelgaas> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240905_094331_910650_AA24DCEC X-CRM114-Status: GOOD ( 38.18 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Andrew Lunn , Catalin Marinas , Michael Turquette , Claudiu Beznea , Eric Dumazet , Dragan Cvetic , Will Deacon , linux-clk@vger.kernel.org, linux-arch@vger.kernel.org, Rob Herring , Florian Fainelli , Lee Jones , Saravana Kannan , Broadcom internal kernel review list , linux-pci@vger.kernel.org, Jakub Kicinski , Paolo Abeni , Linus Walleij , devicetree@vger.kernel.org, Conor Dooley , Arnd Bergmann , linux-gpio@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, Bjorn Helgaas , Andrea della Porta , linux-arm-kernel@lists.infradead.org, Derek Kiernan , Stephen Boyd , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Stefan Wahren , netdev@vger.kernel.org, Krzysztof Kozlowski , "David S. Miller" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Bjorn, On 17:26 Tue 03 Sep , Bjorn Helgaas wrote: > On Mon, Aug 26, 2024 at 09:51:02PM +0200, Andrea della Porta wrote: > > On 10:24 Wed 21 Aug , Bjorn Helgaas wrote: > > > On Tue, Aug 20, 2024 at 04:36:05PM +0200, Andrea della Porta wrote: > > > > The of_pci_set_address() function parses devicetree PCI range > > > > specifier assuming the address is 'sanitized' at the origin, > > > > i.e. without checking whether the incoming address is 32 or 64 > > > > bit has specified in the flags. In this way an address with no > > > > OF_PCI_ADDR_SPACE_MEM64 set in the flags could leak through and > > > > the upper 32 bits of the address will be set too, and this > > > > violates the PCI specs stating that in 32 bit address the upper > > > > bit should be zero. > > > > I don't understand this code, so I'm probably missing something. It > > > looks like the interesting path here is: > > > > > > of_pci_prop_ranges > > > res = &pdev->resource[...]; > > > for (j = 0; j < num; j++) { > > > val64 = res[j].start; > > > of_pci_set_address(..., val64, 0, flags, false); > > > + if (OF_PCI_ADDR_SPACE_MEM64) > > > + prop[1] = upper_32_bits(val64); > > > + else > > > + prop[1] = 0; > > > > > > OF_PCI_ADDR_SPACE_MEM64 tells us about the size of the PCI bus > > > address, but the address (val64) is a CPU physical address, not a PCI > > > bus address, so I don't understand why of_pci_set_address() should use > > > OF_PCI_ADDR_SPACE_MEM64 to clear part of the CPU address. > > > > It all starts from of_pci_prop_ranges(), that is the caller of > > of_pci_set_address(). > > > val64 (i.e. res[j].start) is the address part of a struct resource > > that has its own flags. Those flags are directly translated to > > of_pci_range flags by of_pci_get_addr_flags(), so any > > IORESOURCE_MEM_64 / IORESOURCE_MEM in the resource flag will > > respectively become OF_PCI_ADDR_SPACE_MEM64 / > > OF_PCI_ADDR_SPACE_MEM32 in pci range. > > > What is advertised as 32 bit at the origin (val64) should not become > > a 64 bit PCI address at the output of of_pci_set_address(), so the > > upper 32 bit portion should be dropped. > > > This is explicitly stated in [1] (see page 5), where a space code of 0b10 > > implies that the upper 32 bit of the address must be zeroed out. > > OK, I was confused and thought IORESOURCE_MEM_64 was telling us > something about the *CPU* address, but it's actually telling us > something about what *PCI bus* addresses are possible, i.e., whether > it's a 32-bit BAR or a 64-bit BAR. > > However, the CPU physical address space and the PCI bus address are > not the same. Generic code paths should account for that different by > applying an offset (the offset will be zero on many platforms where > CPU and PCI bus addresses *look* the same). > > So a generic code path like of_pci_prop_ranges() that basically copies > a CPU physical address to a PCI bus address looks broken to me. Hmmm, I'd say that a translation from one bus type to the other is going on nonetheless, and this is done in the current upstream function as well. This patch of course does not add the translation (which is already in place), just to do it avoiding generating inconsistent address. > > Maybe my expectation of this being described in DT is mistaken. Not sure what you mean here, the address being translated are coming from DT, in fact they are described by "ranges" properties. Many thanks, Andrea > Bjorn