Project

General

Profile

Actions

Bug #14001

open

dboot: dump_tables off by one error

Added by Thirteen Oxide 4 months ago. Updated 3 months ago.

Status:
New
Priority:
Low
Assignee:
-
Category:
bootloader
Start date:
Due date:
% Done:

0%

Estimated time:
Difficulty:
Bite-size
Tags:
Gerrit CR:

Description

The pagetable dumping code in dboot's dump_tables() has an off-by-one error that causes kernel VAs to be reported off by 1 GB. It would also cause 1GB worth of kernel mappings to be incorrectly reported as being in identity space. While this code is used only for debugging, it's rather vicious to have your own special debugging code working against you by creating confusion. If we don't intend to use this code at all any more, it should be removed. Otherwise, this should be changed as follows:

diff --git a/usr/src/uts/i86pc/dboot/dboot_startkern.c b/usr/src/uts/i86pc/dboot/dboot_startkern.c
index e6a1127941..10c1e97aec 100644
--- a/usr/src/uts/i86pc/dboot/dboot_startkern.c
+++ b/usr/src/uts/i86pc/dboot/dboot_startkern.c
@@ -527,7 +527,7 @@ dump_tables(void)
                }
 next_entry:
                va += pgsize;
-               if (l == 3 && index == 256)     /* VA hole */
+               if (l == 3 && index == 255)     /* VA hole */
                        va = 0xffff800000000000ull;
 recursion:
                ;
Actions #1

Updated by Toomas Soome 4 months ago

Thirteen Oxide wrote:

The pagetable dumping code in dboot's dump_tables() has an off-by-one error that causes kernel VAs to be reported off by 1 GB. It would also cause 1GB worth of kernel mappings to be incorrectly reported as being in identity space. While this code is used only for debugging, it's rather vicious to have your own special debugging code working against you by creating confusion. If we don't intend to use this code at all any more, it should be removed. Otherwise, this should be changed as follows:

[...]

The idea to remove this code has been around for some time, but this would imply to update boot loaders to load ELF kernel directly and the code for this is not there. Loader we can update, our legacy grub we should drop, and this leaves ipxe boot for smartos...

Actions #2

Updated by Thirteen Oxide 3 months ago

Toomas Soome wrote in #note-1:

The idea to remove this code has been around for some time, but this would imply to update boot loaders to load ELF kernel directly and the code for this is not there. Loader we can update, our legacy grub we should drop, and this leaves ipxe boot for smartos...

I suppose that I should have been clearer: the bug here is not in dboot itself but in a debug function that is used only when specifically requested at boot time. If that debug code specifically is no longer needed, then it should be removed. If it's not going to be removed, this bug should be fixed, because asking for debug output here means you're already having a really bad day and getting the wrong debug output only makes it much worse. My comment wasn't about removing the entirety of dboot in favour of a proper ELF loader, only this debug function. From your note, I assume that dboot is not going away any time soon, so the question is really whether this debugging function is still useful to anyone.

Actions

Also available in: Atom PDF