Bug#249701: runtime checks before jumping into bootstrapped code

[mail_kb@yahoo.com, 249701@bugs.debian.org]

Package: grub
Version: 0.94+cvs20040429-1
Severity: normal
Tags: patch

I'd like to suggest that GRUB do a small sanity check at runtime prior
to loading important sectors and jumping into them. Specifically,
check for magic values when loading the first sector of stage 1.5 or
stage 2, and also when loading the remaining sectors.

This will reduce the chances of GRUB jumping into unknown code when
installation mistakes or other problems cause it to load random data.
e.g. BIOS bugs passing in the wrong drive making it load sectors
from the wrong place, etc.

Would like to offer the following patch to do it, though I'm
afraid it's ugly and shortens an existing error message in order to
squeeze one of the checks into the first sector.

If it helps, be happy to also submit a patch to update the
documentation about the error messages.

Regards,
-kb


diff -ur grub-0.94.orig/stage1/stage1.S grub-0.94/stage1/stage1.S
--- grub-0.94.orig/stage1/stage1.S	Sat Mar 27 09:02:53 2004
+++ grub-0.94/stage1/stage1.S	Tue May 18 13:50:00 2004
@@ -361,6 +361,10 @@
 	movsw
 
 	popw	%ds
+        movw    ABS(stage2_address), %bp
+        cmpw    $S2_OPCODE_MAGIC, (%bp)
+        jne     stage2_notfound_error
+
 	popa
 
 	/* boot stage2 */
@@ -383,6 +387,14 @@
 	jmp	general_error
 
 /*
+ * Didn't find magic bytes at the start of
+ * stage 1.5/2
+ */
+stage2_notfound_error:
+        MSG(stage2_notfound_string)
+        jmp general_error
+
+/*
  * Read error on the disk.
  */
 read_error:
@@ -396,8 +408,9 @@
 
 notification_string:	.string "GRUB "
 geometry_error_string:	.string "Geom"
-hd_probe_error_string:	.string "Hard Disk"
+hd_probe_error_string:	.string "HDsk"
 read_error_string:	.string "Read"
+stage2_notfound_string: .string "S2"
 general_error_string:	.string " Error"
 
 /*
diff -ur grub-0.94.orig/stage1/stage1.h grub-0.94/stage1/stage1.h
--- grub-0.94.orig/stage1/stage1.h	Sat Mar 27 09:02:53 2004
+++ grub-0.94/stage1/stage1.h	Tue May 18 13:49:47 2004
@@ -83,4 +83,9 @@
 /* The drive number of an invalid drive.  */
 #define GRUB_INVALID_DRIVE	0xFF
 
+/* First couple opcodes in stage1.5/2 checked by stage1 before loading */
+#define PUSHW_DX                0x52
+#define PUSHW_SI                0x56
+#define S2_OPCODE_MAGIC         (PUSHW_SI<<8|PUSHW_DX)
+
 #endif /* ! STAGE1_HEADER */
diff -ur grub-0.94.orig/stage2/start.S grub-0.94/stage2/start.S
--- grub-0.94.orig/stage2/start.S	Sun Dec 30 00:23:16 2001
+++ grub-0.94/stage2/start.S	Tue May 18 13:53:39 2004
@@ -63,10 +63,13 @@
 	 */
 	
 	/* save drive reference first thing! */
-	pushw	%dx
+        .byte   PUSHW_DX        /* use constant just to ensure any change
+                                   is propagated to the sanity check
+                                   in stage1.S which verifies this
+                                   opcode is present before jumping here */
 
 	/* print a notification message on the screen */
-	pushw	%si
+	.byte   PUSHW_SI        /* use constant just to ...see above */
 	MSG(notification_string)
 	popw	%si
 	
@@ -309,6 +312,15 @@
 /* END OF MAIN LOOP */
 
 bootit:
+        /* Sanity check we have a reasonable sector */
+#ifdef STAGE1_5
+        movw 0x2206, %dx
+#else /* ! STAGE1_5 */
+        movw 0x8206, %dx
+#endif /* ! STAGE1_5 */
+        cmpw $COMPAT_VERSION, %dx
+        jne bad_stage2_error
+        
 	/* print a newline */
 	MSG(notification_done)
 	popw	%dx	/* this makes sure %dl is our "boot" drive */
@@ -327,6 +339,12 @@
 	jmp	general_error
 
 /*
+ * Rest of stage1.5/2 doesn't have the expected magic value
+ */
+bad_stage2_error:
+        MSG(bad_stage2_error_string)
+        jmp      general_error
+/*
  * Read error on the disk.
  */
 read_error:
@@ -348,6 +366,7 @@
 notification_done:	.string "\r\n"
 	
 geometry_error_string:	.string "Geom"
+bad_stage2_error_string:.string "Header"
 read_error_string:	.string "Read"
 general_error_string:	.string " Error"