From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by mx.groups.io with SMTP id smtpd.web11.435.1670530341008323765 for ; Thu, 08 Dec 2022 12:12:21 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=hY7nZUey; spf=pass (domain: kernel.org, ip: 145.40.73.55, 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 sin.source.kernel.org (Postfix) with ESMTPS id D8284CE244B for ; Thu, 8 Dec 2022 20:12:16 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 24896C433F0 for ; Thu, 8 Dec 2022 20:12:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1670530335; bh=LOsg+6OnKxAOFY+KVl/nIYHK7EVC9hWfWiccwxvXwSQ=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=hY7nZUeylZ2zGR/A73mQZ1socu/Uu4E+7yi64q7yowkugXf7Q5fGiEOWJufeFFJuw d+Y2bl5KdMai77F52x/j23HKhY1eV2Jh5zbak9D0pVs0/lUMVCrpNUUcGqhIo3DH0x 2t3VG1E/KDtkoNaB3yhJYmJVFSXVLg70aQi1fJMZvsVrYRq5YDCUyVaG6N2u433xRV 4A5UQ/dMRelvmDfEVfVOPreXTVm6Vu3Hrh/iSA6cyp8mNxOsQwSM+lIt+LtM9MiFgX kWnLUYXlZeRJo+RRUnr6j0vATOmBqQkZHA8b719qnqb1GObu+t2OggHBtRL6v4J5sy V1GUAcCjDprkA== Received: by mail-lf1-f50.google.com with SMTP id d6so3702310lfs.10 for ; Thu, 08 Dec 2022 12:12:15 -0800 (PST) X-Gm-Message-State: ANoB5plNwFak8Va52GzXpIWRR3byL+S2Z9aC/BwU5y5CZcqEvZO5ZDs2 dAcnRr5hujWEtEGtiRRhGz5g5t+2NIxE6ENvJrw= X-Google-Smtp-Source: AA0mqf7T0JeFCjN3xWK6jw+TBORsWHFu7Yokef0QVY+R//FHa0NI+Rn6Punz1zPontnZVY8AGoybmj89sjPwZE5qzeM= X-Received: by 2002:ac2:5442:0:b0:4b5:1e8:9594 with SMTP id d2-20020ac25442000000b004b501e89594mr19174212lfn.583.1670530333142; Thu, 08 Dec 2022 12:12:13 -0800 (PST) MIME-Version: 1.0 References: <20221207161245.2554193-1-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Thu, 8 Dec 2022 21:12:02 +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 20:20, Kinney, Michael D wrote: > > Hi Ard, > > Much of this code has not been updated since initially added in 2010. > > Looks like a bug to me that has been there the whole time. > > I agree it is a behavior change in the implementation. But unless > new code use of this API looks at the implementation, they would > not know it is destructive and they need to make a copy. This > API is available to external shell apps that use the shell protocol. > Well, not entirely. The function takes EFI_DEVICE_PATH_PROTOCOL** not CONST EFI_DEVICE_PATH_PROTOCOL**, and so one might argue that the underlying object is modifiable by the callee. And similarly, that shell code should not grab a EFI device path protocol pointer from the database and pass it to a function that does not use a CONST qualified EFI_DEVICE_PATH_PROTOCOL pointer to accept the argument. > We should get the shell owners to evaluate removing the destructive > behavior. > I suppose changing the prototypes is out of the question, as doing so would require a new version of the shell protocol? > > -----Original Message----- > > From: Ard Biesheuvel > > Sent: Thursday, December 8, 2022 10:45 AM > > To: Kinney, Michael D > > Cc: devel@edk2.groups.io; Ni, Ray ; Gao, Zhichao > > Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols > > > > On Thu, 8 Dec 2022 at 19:28, Kinney, Michael D > > wrote: > > > > > > Hi Ard, > > > > > > From this description, it does not look like it should be modifying the > > > contents of the device path. Just point to the device path end node that > > > follows the match found. > > > > > > /** > > > Gets the mapping that most closely matches the device path. > > > > > > This function gets the mapping which corresponds to the device path *DevicePath. If > > > there is no exact match, then the mapping which most closely matches *DevicePath > > > is returned, and *DevicePath is updated to point to the remaining portion of the > > > device path. If there is an exact match, the mapping is returned and *DevicePath > > > points to the end-of-device-path node. > > > > > > @param DevicePath On entry, points to a device path pointer. On > > > exit, updates the pointer to point to the > > > portion of the device path after the mapping. > > > > > > @retval NULL No mapping was found. > > > @return !=NULL Pointer to NULL-terminated mapping. The buffer > > > is callee allocated and should be freed by the caller. > > > **/ > > > CONST CHAR16 * > > > EFIAPI > > > EfiShellGetMapFromDevicePath ( > > > IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath > > > ); > > > > > > I see this API used in many places, and it looks like it would be > > > destructive to any multi-instance device path. Multi-instance > > > device paths are typically used for consoles, so we may not have > > > noticed this destructive behavior with file system mapping paths. > > > > > > Did you try removing the call to SetDevicePathEndNode (*DevicePath); ? > > > > > > > No, but that would be a functional change visible to all users of the > > current API. > > > > And note that the calling code already has 'DevicePathCopy' variables, > > it just doesn't bother using them, so the intent is clearly to pass a > > copy, not the actual device path.