.../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 83 +++++++++++++++---- 1 file changed, 68 insertions(+), 15 deletions(-)
The new ArmMmuLib code is easier to reason about, so that is what I did: currently, when we create mappings that cover existing table entries, we may end up overwriting those with block entries without taking the mapping attributes of the original table entries into account. So let's fix this. I honestly don't know whether the original code was better at dealing with this: I do remember some changes from Heyi that may have been related, but the old code is not easy to follow. In any case, I didn't manage to hit this case in practice, given that we typically start out with large mappings, and break them down later (to set permissions), rather than the other way around. Patch #1 adds some helpers to hide the insane way the type bits change meaning when you change to level 3. Patch #2 ensures that we only replace (and free) table entries with block entries if it is guaranteed that doing so will not lose any attribute information. Changes since v2: - add patch to limit recursion to levels < 3 in FreePageTablesRecursive() Changes since v1: - zero newly allocated pages before splitting a block entry into a table entry, to avoid garbage in that page being misidentified as entry type attributes - this should fix the crash observed by Laszlo Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif@nuviainc.com> Cc: Ashish Singhal <ashishsingha@nvidia.com> Ard Biesheuvel (3): ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 83 +++++++++++++++---- 1 file changed, 68 insertions(+), 15 deletions(-) -- 2.17.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56300): https://edk2.groups.io/g/devel/message/56300 Mute This Topic: https://groups.io/mt/72543071/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 03/25/20 16:29, Ard Biesheuvel wrote: > The new ArmMmuLib code is easier to reason about, so that is what I did: > currently, when we create mappings that cover existing table entries, we > may end up overwriting those with block entries without taking the mapping > attributes of the original table entries into account. So let's fix this. > > I honestly don't know whether the original code was better at dealing with > this: I do remember some changes from Heyi that may have been related, but > the old code is not easy to follow. In any case, I didn't manage to hit this > case in practice, given that we typically start out with large mappings, and > break them down later (to set permissions), rather than the other way around. > > Patch #1 adds some helpers to hide the insane way the type bits change > meaning when you change to level 3. > > Patch #2 ensures that we only replace (and free) table entries with block > entries if it is guaranteed that doing so will not lose any attribute > information. > > Changes since v2: > - add patch to limit recursion to levels < 3 in FreePageTablesRecursive() > > Changes since v1: > - zero newly allocated pages before splitting a block entry into a table > entry, to avoid garbage in that page being misidentified as entry type > attributes - this should fix the crash observed by Laszlo > > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif@nuviainc.com> > Cc: Ashish Singhal <ashishsingha@nvidia.com> > > Ard Biesheuvel (3): > ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables > ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types > ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table > entry > > .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 83 +++++++++++++++---- > 1 file changed, 68 insertions(+), 15 deletions(-) > Tested-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56340): https://edk2.groups.io/g/devel/message/56340 Mute This Topic: https://groups.io/mt/72543071/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Wed, 25 Mar 2020 at 20:49, Laszlo Ersek <lersek@redhat.com> wrote: > > On 03/25/20 16:29, Ard Biesheuvel wrote: > > The new ArmMmuLib code is easier to reason about, so that is what I did: > > currently, when we create mappings that cover existing table entries, we > > may end up overwriting those with block entries without taking the mapping > > attributes of the original table entries into account. So let's fix this. > > > > I honestly don't know whether the original code was better at dealing with > > this: I do remember some changes from Heyi that may have been related, but > > the old code is not easy to follow. In any case, I didn't manage to hit this > > case in practice, given that we typically start out with large mappings, and > > break them down later (to set permissions), rather than the other way around. > > > > Patch #1 adds some helpers to hide the insane way the type bits change > > meaning when you change to level 3. > > > > Patch #2 ensures that we only replace (and free) table entries with block > > entries if it is guaranteed that doing so will not lose any attribute > > information. > > > > Changes since v2: > > - add patch to limit recursion to levels < 3 in FreePageTablesRecursive() > > > > Changes since v1: > > - zero newly allocated pages before splitting a block entry into a table > > entry, to avoid garbage in that page being misidentified as entry type > > attributes - this should fix the crash observed by Laszlo > > > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Leif Lindholm <leif@nuviainc.com> > > Cc: Ashish Singhal <ashishsingha@nvidia.com> > > > > Ard Biesheuvel (3): > > ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables > > ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types > > ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table > > entry > > > > .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 83 +++++++++++++++---- > > 1 file changed, 68 insertions(+), 15 deletions(-) > > > > Tested-by: Laszlo Ersek <lersek@redhat.com> > Pushed to master Thanks all -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56393): https://edk2.groups.io/g/devel/message/56393 Mute This Topic: https://groups.io/mt/72543071/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Patch set version 3 looks good to me. I have reviewed and tested the changes locally. Reviewed-by: Ashish Singhal <ashishsingha@nvidia.com> Tested-by: Ashish Singhal <ashishsingha@nvidia.com> Thanks Ashish ________________________________ From: Ard Biesheuvel <ard.biesheuvel@linaro.org> Sent: Wednesday, March 25, 2020 9:29 AM To: devel@edk2.groups.io <devel@edk2.groups.io> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Ashish Singhal <ashishsingha@nvidia.com> Subject: [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix External email: Use caution opening links or attachments The new ArmMmuLib code is easier to reason about, so that is what I did: currently, when we create mappings that cover existing table entries, we may end up overwriting those with block entries without taking the mapping attributes of the original table entries into account. So let's fix this. I honestly don't know whether the original code was better at dealing with this: I do remember some changes from Heyi that may have been related, but the old code is not easy to follow. In any case, I didn't manage to hit this case in practice, given that we typically start out with large mappings, and break them down later (to set permissions), rather than the other way around. Patch #1 adds some helpers to hide the insane way the type bits change meaning when you change to level 3. Patch #2 ensures that we only replace (and free) table entries with block entries if it is guaranteed that doing so will not lose any attribute information. Changes since v2: - add patch to limit recursion to levels < 3 in FreePageTablesRecursive() Changes since v1: - zero newly allocated pages before splitting a block entry into a table entry, to avoid garbage in that page being misidentified as entry type attributes - this should fix the crash observed by Laszlo Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif@nuviainc.com> Cc: Ashish Singhal <ashishsingha@nvidia.com> Ard Biesheuvel (3): ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 83 +++++++++++++++---- 1 file changed, 68 insertions(+), 15 deletions(-) -- 2.17.1 ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ----------------------------------------------------------------------------------- -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56307): https://edk2.groups.io/g/devel/message/56307 Mute This Topic: https://groups.io/mt/72543071/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.