From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.4443.1670539057238524387 for ; Thu, 08 Dec 2022 14:37:37 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=kGEMpcvp; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id BB5BD620A2 for ; Thu, 8 Dec 2022 22:37:36 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 270D7C433F0 for ; Thu, 8 Dec 2022 22:37:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1670539056; bh=6t3fK5c3MW1Q7r9ckcS0jUzRKIC4gTzvDcpaAglHGSk=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=kGEMpcvpkI6yNCIs+CU0f1ZJxFdPOUnZaXgNDDOkn+dA47yGCFatI/TtfkNThgvyQ os/x4KdVkMpBx8JTFH7VqV2Ne5hdQAkrHarcFYoix4LNHJ/eHs36P2cSsW6/LEs+Fp GQwDE8xI1BOfmMjtFQzewC19JoMwv/E6Q14WDWCWpJXwRsYRIM/8j5lW3Viet/eeOu g6FCqO1gaHDlROsDTZ/XoNSnYDVu1g5SA8DXG0dVm/1tWpaPducTr4/BCTCyVEFu24 3my5QsNyJCPkAEKSwwvygyo2bSgpTlfZfH00Z9HHN3ugZ9q28FT2jWKPGmGnhvnaiv 0BhdmBvJaWi6A== Received: by mail-lf1-f53.google.com with SMTP id g7so4262082lfv.5 for ; Thu, 08 Dec 2022 14:37:36 -0800 (PST) X-Gm-Message-State: ANoB5plyuBf3X/1sEXxrLR7vAD2WBSLSpxL2lxxFDHKdxSRWr5Kr2+fu Bpe6kT/CuWeIvgZ8eCDaShQKCzb4u4n/JQTl7K4= X-Google-Smtp-Source: AA0mqf7B0bVmcQx3ZoxWi9FAh1DZGu7Jnua6Jl1ldO0ctRYlfXooThQVImKmpW3B1X7LN6fHMc2QNNFDd5jnWMvaR40= X-Received: by 2002:a05:6512:2282:b0:4b4:f88b:844 with SMTP id f2-20020a056512228200b004b4f88b0844mr22167416lfu.539.1670539054118; Thu, 08 Dec 2022 14:37:34 -0800 (PST) MIME-Version: 1.0 References: <20221207161245.2554193-1-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Thu, 8 Dec 2022 23:37:22 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols To: "Kinney, Michael D" Cc: "devel@edk2.groups.io" , "Ni, Ray" , "Gao, Zhichao" Content-Type: text/plain; charset="UTF-8" On Thu, 8 Dec 2022 at 23:35, Ard Biesheuvel wrote: > > On Thu, 8 Dec 2022 at 22:57, Kinney, Michael D > wrote: > > > > Ard, > > > > Thank you for the correction. > > > > If we add that CONST, then the ShellPkg build breaks with an error > > > > c:\work\github\tianocore\edk2\ShellPkg\Application\Shell\ShellProtocol.c(297): error C2220: the following warning is treated as an error > > c:\work\github\tianocore\edk2\ShellPkg\Application\Shell\ShellProtocol.c(297): warning C4090: 'function': different 'const' qualifiers > > > > Which is exactly the line we want to remove to prevent the destructive behavior. > > > > SetDevicePathEndNode (*DevicePath); > > > > If I comment out that line, the ShellPkg build completes with no errors. > > > > I'm surprised that this is the only offending line, and I suppose that > is good news. > > But as you said, the shell protocol is used much more widely, and > existing callers may rely on the destructive behavior. > > At the very least, we should only perform the SetDevicePathEndNode() > call if the node in question is not already an > end-of-entire-devicepath node, as the update is pointless in that > case, but will still trigger a fault if the memory is read-only. > > But it really depends on whether any callers might exist that expect a > multi-instance devicepath to be split up. > > > I agree that it would be better to update the prototype and get help > > from the compiler to find incorrect implementations. Even though > > CONST is not in the prototype, from reading the description of the API > > it does not state that the contents are modified, so I think the > > intent was no modifications. > > > > Agreed. > > Your suggested change is safe, but it is incomplete because there > > are additional calls through the protocol that are not covered > > by your patch. We also do not know how many places this API > > is used in downstream projects. This side effect of a write to > > a read-only page and potential corruption of a multi-instance > > device path looks like a bug to me and we should fix the root > > cause and not fix just some of the callers. > > > > OK, so what is the way forward here? > I sent this before I noticed your other reply. So let's go with your fix to preserve the existing behavior without triggering the fault.