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.web10.4387.1670538969074202325 for ; Thu, 08 Dec 2022 14:36:09 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=gLyNVkpb; 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 7424B62088 for ; Thu, 8 Dec 2022 22:36:08 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D8C47C433F0 for ; Thu, 8 Dec 2022 22:36:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1670538967; bh=W9fbSIgWn7Qn3RhMI3B9K4KapyqTF5bzw49oowSrIFw=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=gLyNVkpbawgaKisGrI9NEdmuX4uqk43WZU+fuLvjn+fLXLt9Ct+iBXiu0S5UQ4bLf WDGqA2W5j4F1oRQQ4qggbHEA4IuESSEZtafn6M+IAcGli44cPNtrMJDuIQ4dn542sz 0ZQdQFB1Qu8sGS92Hm06ntlr17SgQN8PqCQ3UqC2JgFAmm7+RaYsxoqXZ3j4qOL38G bbfrXGjG8u1wbt34mroodT8ja+80NUx/WAnDnIPwzoWyv04xJ5FULGVJ5YJIWGplnM kcTcPUimK7Dq3XoV64CYjyxFZNHX/CwVsg5mAmiTCInZoe4lIWDTNTxDpz32WUWXfs tFlpclT+s4bGg== Received: by mail-lf1-f47.google.com with SMTP id j4so4301639lfk.0 for ; Thu, 08 Dec 2022 14:36:07 -0800 (PST) X-Gm-Message-State: ANoB5pm6P+GGqlCNj1vPauHWsx8qZrUPNHM/frCz0Fv4vtY7/KnbeEEv stWygnZ5tJMHhvLT7BlsBZX0X7NfRbskFcslnkc= X-Google-Smtp-Source: AA0mqf6GMFazWE+owmMoeX3pAXnoCW6IQWhSsX6T9bMmLqbgbn70+728bfUDPK1zKfk3Lt43ss3GGYCe+ljuQ1wFU1U= X-Received: by 2002:a05:6512:3c89:b0:4a2:bfd2:b218 with SMTP id h9-20020a0565123c8900b004a2bfd2b218mr31677948lfv.228.1670538965917; Thu, 08 Dec 2022 14:36:05 -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:35:54 +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 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? > > -----Original Message----- > > From: devel@edk2.groups.io On Behalf Of Ard Biesheuvel > > Sent: Thursday, December 8, 2022 1:40 PM > > To: devel@edk2.groups.io; Kinney, Michael D > > Cc: Ni, Ray ; Gao, Zhichao > > Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols > > > > On Thu, 8 Dec 2022 at 22:15, Michael D Kinney > > wrote: > > > > > > Hi Ard, > > > > > > There is a difference between returning a pointer to a device path > > > and modifying the device path contents. > > > > > > If you add CONST to the argument, then an updated pointer to a device > > > path can not be returned. > > > > > > > No, this is incorrect. > > > > The function takes a pointer (1) to a pointer(2) to a device path protocol > > > > EFI_DEVICE_PATH_PROTOCOL ** > > > > So the function can dereference pointer 1 and modify pointer 2 > > *unless* it is marked as CONST, i.e. > > > > EFI_DEVICE_PATH_PROTOCOL * CONST * > > > > in which case the pointer is not modifiable, but it is permitted to > > dereference that pointer to modify the underlying object. > > > > I am arguing that the prototype should be > > > > EFI_DEVICE_PATH_PROTOCOL CONST ** > > > > (which is the same as putting the CONST at the beginning) > > > > where the caller's pointer can be advanced by the callee via the > > pointer-to-pointer. But that would still not permit the object to be > > modified. > > > > > The API clear describes returning an updated device path pointer, so > > > the API is declared correctly without CONST. > > > > > > > The pointer may be updated but not the object. It really comes down to > > the difference between > > > > CONST EFI_DEVICE_PATH_PROTOCOL ** > > EFI_DEVICE_PATH_PROTOCOL CONST ** > > EFI_DEVICE_PATH_PROTOCOL * CONST * > > EFI_DEVICE_PATH_PROTOCOL **CONST > > > > (where the first two mean the same thing0 > > > > > The API does not state that the contents of the device path are modified. > > > > > > An API that uses CONST EFI_DEVICE_PATH* would indicate that the API > > > should not modify the contents of the device path. For example: > > > > > > /** > > > Returns the size of a device path in bytes. > > > > > > This function returns the size, in bytes, of the device path data structure > > > specified by DevicePath including the end of device path node. > > > If DevicePath is NULL or invalid, then 0 is returned. > > > > > > @param DevicePath A pointer to a device path data structure. > > > > > > @retval 0 If DevicePath is NULL or invalid. > > > @retval Others The size of a device path in bytes. > > > > > > **/ > > > UINTN > > > EFIAPI > > > GetDevicePathSize ( > > > IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath > > > ); > > > > > > > Yes, but this one is a pointer, not a pointer-to-pointer. Big difference. > > > > > > > > >