[PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[Alon Bar-Lev]

[-- Attachment #1: Type: text/plain, Size: 2332 bytes --]


Extending the kernel parameters to a user determined size
on boot protocol >=2.02 for i386 and x86_64 architectures.

Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>

---

Hello,

Current implementation allows the kernel to receive up to
255 characters from the bootloader.

In current environment, the command-line is used in order to
specify many values, including suspend/resume, module
arguments, splash, initramfs and more.

255 characters are not enough anymore.

This is take 4 of this submission.

Take 4 (current), no reply of what is the problem in LILO.
Can someone please reply Peter questions? I understand that
people don't want the new kernel config option... But fixing
LILO should be more complex.

Take 3 - Back to menuconfig option, so it won't
break LILO (Although if it is broken because of this, it
should be fixed...).

Here are the comments from H. Peter Anvin:
> Does anyone know the actual details of the LILO breakage?
> If the problem is that LILO doesn't null-terminate the string when it's too long,
> then we can deal with this automatically, without introducing compile-time options
> (which were already once rejected.)
> 
> If the problem is with LILO booting *old* kernels, then that's going to have to require
> some LILO changes, and probably a boot revision bump.

Take 2 - Removed the menuconfig into a fixed 2048 size. This
patch was rejected. This is what the 2.6.11-rc2 changelog
has to say about the matter:

> Revert "x86_64/i386: increase command line size" patch
> It's a bootup dependancy, you can't just increase it
> randomly, and it breaks booting with LILO.
> Pointed out by Janos Farkas and Adrian Bunk.

Take 1 - Patch that allows command-line size to be
determined in menuconfig. People did not want to see a new
config option.

---

If any of you think that this should be applied for other
architectures, please reply.

Current architectures that have 256 limit are:
alpha, cris, i386, m64k, mips, sh, sh64, sparc, sparc64,
x86_64, xtensa

Current architectures that have 512 values are:
frv(512), h8300(512), ia64(512), m32r(512), m68knommu(512),
mips(512), powerpc(512), v850(512)

Current architectures that are OK:
arm(1024), arm26(1024), parisc(1024)

Current strange ones:
s390(896) - I guess IBM has a good reason for this constant...

Best Regards,
Alon Bar-Lev


[-- Attachment #2: linux-2.6.16-x86-command-line.diff --]
[-- Type: text/plain, Size: 2847 bytes --]

diff -urNp linux-2.6.16/arch/i386/Kconfig linux-2.6.16.new/arch/i386/Kconfig
--- linux-2.6.16/arch/i386/Kconfig	2006-03-20 07:53:29.000000000 +0200
+++ linux-2.6.16.new/arch/i386/Kconfig	2006-04-14 01:35:06.000000000 +0300
@@ -644,6 +644,14 @@ config EFI
 	anything about EFI).  However, even with this option, the resultant
 	kernel should continue to boot on existing non-EFI platforms.
 
+config COMMAND_LINE_SIZE
+	int "Maximum kernel command-line size"
+	range 256 4096
+	default 256
+	---help---
+	This enables adjusting maximum command-line size. If you are unsure
+	specify 256.
+
 config IRQBALANCE
  	bool "Enable kernel irq balancing"
 	depends on SMP && X86_IO_APIC
diff -urNp linux-2.6.16/arch/x86_64/Kconfig linux-2.6.16.new/arch/x86_64/Kconfig
--- linux-2.6.16/arch/x86_64/Kconfig	2006-03-20 07:53:29.000000000 +0200
+++ linux-2.6.16.new/arch/x86_64/Kconfig	2006-04-14 01:35:30.000000000 +0300
@@ -445,6 +445,14 @@ config PHYSICAL_START
 
 	  Don't change this unless you know what you are doing.
 
+config COMMAND_LINE_SIZE
+	int "Maximum kernel command-line size"
+	range 256 4096
+	default 256
+	---help---
+	This enables adjusting maximum command-line size. If you are unsure
+	specify 256.
+
 config SECCOMP
 	bool "Enable seccomp to safely compute untrusted bytecode"
 	depends on PROC_FS
diff -urNp linux-2.6.16/include/asm-i386/param.h linux-2.6.16.new/include/asm-i386/param.h
--- linux-2.6.16/include/asm-i386/param.h	2006-03-20 07:53:29.000000000 +0200
+++ linux-2.6.16.new/include/asm-i386/param.h	2006-04-14 02:00:45.000000000 +0300
@@ -19,6 +19,15 @@
 #endif
 
 #define MAXHOSTNAMELEN	64	/* max length of hostname */
+
+/*
+ * This COMMAND_LINE_SIZE definition was left here
+ * for compatability, its correct location is in setup.h.
+ * Boot loaders that use this parameters will continue
+ * to use 256 maximum command-line size.
+ */
+#ifndef CONFIG_COMMAND_LINE_SIZE
 #define COMMAND_LINE_SIZE 256
+#endif
 
 #endif
diff -urNp linux-2.6.16/include/asm-i386/setup.h linux-2.6.16.new/include/asm-i386/setup.h
--- linux-2.6.16/include/asm-i386/setup.h	2006-03-20 07:53:29.000000000 +0200
+++ linux-2.6.16.new/include/asm-i386/setup.h	2006-04-14 01:32:16.000000000 +0300
@@ -17,7 +17,7 @@
 #define MAX_NONPAE_PFN	(1 << 20)
 
 #define PARAM_SIZE 4096
-#define COMMAND_LINE_SIZE 256
+#define COMMAND_LINE_SIZE CONFIG_COMMAND_LINE_SIZE
 
 #define OLD_CL_MAGIC_ADDR	0x90020
 #define OLD_CL_MAGIC		0xA33F
diff -urNp linux-2.6.16/include/asm-x86_64/setup.h linux-2.6.16.new/include/asm-x86_64/setup.h
--- linux-2.6.16/include/asm-x86_64/setup.h	2006-03-20 07:53:29.000000000 +0200
+++ linux-2.6.16.new/include/asm-x86_64/setup.h	2006-04-14 01:33:27.000000000 +0300
@@ -1,6 +1,6 @@
 #ifndef _x8664_SETUP_H
 #define _x8664_SETUP_H
 
-#define COMMAND_LINE_SIZE	256
+#define COMMAND_LINE_SIZE	CONFIG_COMMAND_LINE_SIZE
 
 #endif

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[H. Peter Anvin]

Alon Bar-Lev wrote:
> 
>> Revert "x86_64/i386: increase command line size" patch
>> It's a bootup dependancy, you can't just increase it
>> randomly, and it breaks booting with LILO.
>> Pointed out by Janos Farkas and Adrian Bunk.
> 

Can we get the details, please, instead of a repeat of the same patch, 
so we can figure out a proper solution?

	-hpa

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[Alon Bar-Lev]

H. Peter Anvin wrote:
> Alon Bar-Lev wrote:
>>
>>> Revert "x86_64/i386: increase command line size" patch
>>> It's a bootup dependancy, you can't just increase it
>>> randomly, and it breaks booting with LILO.
>>> Pointed out by Janos Farkas and Adrian Bunk.
>>
> 
> Can we get the details, please, instead of a repeat of the same patch,
> so we can figure out a proper solution?
> 
>     -hpa
> 

Hello Peter,

I don't know any other way to get the details. I am truly
thank you for your responses. But the people that rejected
this patch gave no detailed reason! I've extended the CC
list this time in a hope that someone will reply.

I also specify the exact history for this issue... In order
to encourage relevant people to reply.

This should be a simple modification and I don't see why we
should fight on the LILO problem (if exists) when we have
the compile time config options alternative.

People who uses LILO may leave the default 256 value. Other
may migrate to a higher one.

I also don't understand why every architecture have a
different command line size... The compile time config
option may solve all this to a unified solution with
different default for every architecture.

I will be glad to learn of a better to push this matter
forward, without adding a LILO specific code into the
kernel, which I don't think is wise... I prefer to continue
patching my and others kernel and not mess up kernel with
LILO specific code.

If you think I should stop this effort, please say so... I
will drop it.

Best Regards,
Alon Bar-Lev.

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[H. Peter Anvin]

Alon Bar-Lev wrote:
> 
> This should be a simple modification and I don't see why we
> should fight on the LILO problem (if exists) when we have
> the compile time config options alternative.
> 
> People who uses LILO may leave the default 256 value. Other
> may migrate to a higher one.
> 

This is cargo-cult programming, sorry.  Let's find out what the problem 
is and fix it right.  If that involves fixing LILO and/or dealing with a 
LILO bug, we have ways we can do that.

	-hpa

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[John Coffman]

It is probably fairly easy to increase the LILO command line to 512 
bytes (including terminator).  Beyond 512 there are complicating factors:

1.  "lilo -R ..." -- the space reserved for the stored command line 
is 1 sector.
2.  configuration option "fallback" -- again 1 sector is the amount reserved.

There are 2 buffers used for the command line.  Since these are 
allocated on sector boundaries, 512 should present no serious problems.

--John


At 07:35 AM  Friday 5/5/2006, H. Peter Anvin wrote:
>Alon Bar-Lev wrote:
>>This should be a simple modification and I don't see why we
>>should fight on the LILO problem (if exists) when we have
>>the compile time config options alternative.
>>People who uses LILO may leave the default 256 value. Other
>>may migrate to a higher one.
>
>This is cargo-cult programming, sorry.  Let's find out what the 
>problem is and fix it right.  If that involves fixing LILO and/or 
>dealing with a LILO bug, we have ways we can do that.
>
>         -hpa


         PGP KeyID: 6781C9C8  (good until 31-Dec-2008)
         Keyserver at  ldap://keyserver.pgp.com  OR  http://pgp.mit.edu
         LILO links at http://freshmeat.net/projects/lilo
         and Help link at http://lilo.go.dyndns.org

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[H. Peter Anvin]

John Coffman wrote:
> It is probably fairly easy to increase the LILO command line to 512 
> bytes (including terminator).  Beyond 512 there are complicating factors:
> 
> 1.  "lilo -R ..." -- the space reserved for the stored command line is 1 
> sector.
> 2.  configuration option "fallback" -- again 1 sector is the amount 
> reserved.
> 
> There are 2 buffers used for the command line.  Since these are 
> allocated on sector boundaries, 512 should present no serious problems.
> 

The problem isn't that LILO can't handle more than some number of characters; that's a 
LILO issue and doesn't affect the kernel.

The problem is that some people have reported that the kernel crashes if booted with LILO 
and the size limit is more than 255.  They haven't so far commented on how they observed 
that, and that's a major problem.

If the issue is that LILO doesn't null-terminate overlong command lines, then that's 
pretty easy to deal with:

- If the kernel sees protocol version <= 2.01, limit is 255+null.
- If the kernel sees protocol version >= 2.02, but ID is 0x1X, limit is 255+null.
- Otherwise limit is higher.

When LILO is fixed, it has to bump the ID byte version number.

What ID byte values has LILO used?

	-hpa

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[John Coffman]

At 11:17 AM  Friday 5/5/2006, you wrote:
>John Coffman wrote:
>The problem isn't that LILO can't handle more than some number of 
>characters; that's a LILO issue and doesn't affect the kernel.
>
>The problem is that some people have reported that the kernel 
>crashes if booted with LILO and the size limit is more than 
>255.  They haven't so far commented on how they observed that, and 
>that's a major problem.

Just re-compiling LILO with the  COMMAND_LINE_SIZE  parameter changed 
from 256 to 512 will not work.  A .bss area must be moved to avoid 
clobbering the kernel header.


>If the issue is that LILO doesn't null-terminate overlong command 
>lines, then that's pretty easy to deal with:
>
>- If the kernel sees protocol version <= 2.01, limit is 255+null.
>- If the kernel sees protocol version >= 2.02, but ID is 0x1X, limit 
>is 255+null.
>- Otherwise limit is higher.
>
>When LILO is fixed, it has to bump the ID byte version number.
>
>What ID byte values has LILO used?

For the last 8 years LILO has used 0x02 as the loader ID.

If anyone wishes to test a version of LILO that is able to pass a 512 
byte command line, then the "22.7.2-beta8" version in the "beta" 
directory should be tried.  It moves the offending ".bss" area to 
avoid the header clobber.  However, I have not yet changed the loader ID.

--John



         PGP KeyID: 6781C9C8  (good until 31-Dec-2008)
         Keyserver at  ldap://keyserver.pgp.com  OR  http://pgp.mit.edu
         LILO links at http://freshmeat.net/projects/lilo
         and Help link at http://lilo.go.dyndns.org

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[H. Peter Anvin]

John Coffman wrote:
>>
>> The problem is that some people have reported that the kernel crashes 
>> if booted with LILO and the size limit is more than 255.  They haven't 
>> so far commented on how they observed that, and that's a major problem.
> 
> Just re-compiling LILO with the  COMMAND_LINE_SIZE  parameter changed 
> from 256 to 512 will not work.  A .bss area must be moved to avoid 
> clobbering the kernel header.
> 

Okay, let me ask this:

If the *kernel* limit is modified, but the LILO limit is not, what will happen?  This is 
the real crux of the matter.

	-hpa

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[Alon Bar-Lev]

John Coffman wrote:
> Just re-compiling LILO with the  COMMAND_LINE_SIZE  parameter changed
> from 256 to 512 will not work.  A .bss area must be moved to avoid
> clobbering the kernel header.

Hello John,

The COMMAND_LINE_SIZE should be fixed 256 bytes for boot
protocol < 2.02.

For boot protocol >= 2.02 it can be null terminated 256 and up.

>From LILO code I can see that COMMAND_LINE_SIZE is defined
in lilo.h, so I don't understand how a change in the
COMMAND_LINE_SIZE of the kernel affect LILO.

What we want to achieve is a kernel capable of accepting
command line size greater than 256 bytes... It is OK if LILO
will still pass 256 byte buffer as it already does.

Can you think of a reason why LILO will not work if we do
that? (lilo.h keeps #define COMMAND_LINE_SIZE 256).

Best Regards,
Alon Bar-Lev.

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[John Coffman]

At 02:57 PM  Friday 5/5/2006, H. Peter Anvin wrote:
Okay, let me ask this:

>If the *kernel* limit is modified, but the LILO limit is not, what 
>will happen?  This is the real crux of the matter.

The length of the kernel command line will be limited by the size of 
the boot loader buffer.  LILO always inserts a NUL terminator.

--John

P.S.  The LILO command line buffer has always been 1 sector (512 
bytes); however, only the first half is actually used for the command 
line. No kernel can do any harm by setting "boot_cmdline[511] = 0;" 
for any version of LILO back to version 20 (and probably before).



         PGP KeyID: 6781C9C8  (good until 31-Dec-2008)
         Keyserver at  ldap://keyserver.pgp.com  OR  http://pgp.mit.edu
         LILO links at http://freshmeat.net/projects/lilo
         and Help link at http://lilo.go.dyndns.org

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[H. Peter Anvin]

John Coffman wrote:
> At 02:57 PM  Friday 5/5/2006, H. Peter Anvin wrote:
> Okay, let me ask this:
> 
>> If the *kernel* limit is modified, but the LILO limit is not, what 
>> will happen?  This is the real crux of the matter.
> 
> The length of the kernel command line will be limited by the size of the 
> boot loader buffer.  LILO always inserts a NUL terminator.
> 
> --John
> 
> P.S.  The LILO command line buffer has always been 1 sector (512 bytes); 
> however, only the first half is actually used for the command line. No 
> kernel can do any harm by setting "boot_cmdline[511] = 0;" for any 
> version of LILO back to version 20 (and probably before).
> 

Okay... **DOES ANYONE HAVE ANY ACTUAL EVIDENCE TO THE CONTRARY???**, and 
if so, **WHAT ARE THE DETAILS**?

All I've heard so far is hearsay.  "X said that Y had said..."

	-hpa

Re: [PATCH][TAKE 4] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit

[Alon Bar-Lev]

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

H. Peter Anvin wrote:
> John Coffman wrote:
>> At 02:57 PM  Friday 5/5/2006, H. Peter Anvin wrote:
>> Okay, let me ask this:
>>
>>> If the *kernel* limit is modified, but the LILO limit is not, what
>>> will happen?  This is the real crux of the matter.
>>
>> The length of the kernel command line will be limited by the size of
>> the boot loader buffer.  LILO always inserts a NUL terminator.
>>
>> --John
>>
>> P.S.  The LILO command line buffer has always been 1 sector (512
>> bytes); however, only the first half is actually used for the command
>> line. No kernel can do any harm by setting "boot_cmdline[511] = 0;"
>> for any version of LILO back to version 20 (and probably before).
>>
> 
> Okay... **DOES ANYONE HAVE ANY ACTUAL EVIDENCE TO THE CONTRARY???**, and
> if so, **WHAT ARE THE DETAILS**?
> 
> All I've heard so far is hearsay.  "X said that Y had said..."
> 
>     -hpa
> 
> 

So here is an updated patch. Notice that I've removed the
redundant COMMAND_LINE_SIZE from param.h of i386 to make it
closer to other architectures. It was required in the past
to allow a boot loader to know the COMMAND_LINE_SIZE, but
LILO, GRUB, syslinux have a local definition for this, and
is not required in boot protocol >= 2.02 since a boot loader
can pass any null terminated string.

Best Regards,
Alon Bar-Lev.

[-- Attachment #2: linux-2.6.17-rc3-x86-command-line.patch --]
[-- Type: text/plain, Size: 1274 bytes --]

diff -urNp linux-2.6.17-rc3/include/asm-i386/param.h linux-2.6.17-rc3.new/include/asm-i386/param.h
--- linux-2.6.17-rc3/include/asm-i386/param.h	2006-03-20 07:53:29.000000000 +0200
+++ linux-2.6.17-rc3.new/include/asm-i386/param.h	2006-05-06 12:38:32.000000000 +0300
@@ -19,6 +19,5 @@
 #endif
 
 #define MAXHOSTNAMELEN	64	/* max length of hostname */
-#define COMMAND_LINE_SIZE 256
 
 #endif
diff -urNp linux-2.6.17-rc3/include/asm-i386/setup.h linux-2.6.17-rc3.new/include/asm-i386/setup.h
--- linux-2.6.17-rc3/include/asm-i386/setup.h	2006-05-06 12:35:09.000000000 +0300
+++ linux-2.6.17-rc3.new/include/asm-i386/setup.h	2006-05-06 12:38:44.000000000 +0300
@@ -15,7 +15,7 @@
 #define MAX_NONPAE_PFN	(1 << 20)
 
 #define PARAM_SIZE 4096
-#define COMMAND_LINE_SIZE 256
+#define COMMAND_LINE_SIZE 2048
 
 #define OLD_CL_MAGIC_ADDR	0x90020
 #define OLD_CL_MAGIC		0xA33F
diff -urNp linux-2.6.17-rc3/include/asm-ia64/setup.h linux-2.6.17-rc3.new/include/asm-ia64/setup.h
--- linux-2.6.17-rc3/include/asm-ia64/setup.h	2006-03-20 07:53:29.000000000 +0200
+++ linux-2.6.17-rc3.new/include/asm-ia64/setup.h	2006-05-06 12:40:32.000000000 +0300
@@ -1,6 +1,6 @@
 #ifndef __IA64_SETUP_H
 #define __IA64_SETUP_H
 
-#define COMMAND_LINE_SIZE	512
+#define COMMAND_LINE_SIZE	2048
 
 #endif

[PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

Extending the kernel parameters to a 2048 bytes for
boot protocol >=2.02 of i386, ia64 and x86_64 architectures for
linux-2.6.18-rc4-mm2.

Current implementation allows the kernel to receive up to
255 characters from the bootloader. In current environment,
the command-line is used in order to specify many values,
including suspend/resume, module arguments, splash, initramfs
and more. 255 characters are not enough anymore.

Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>

---

diff -urNp linux-2.6.18-rc4-mm2/include/asm-i386/param.h linux-2.6.18-rc4-mm2.new/include/asm-i386/param.h
--- linux-2.6.18-rc4-mm2/include/asm-i386/param.h	2006-08-25 16:10:56.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/include/asm-i386/param.h	2006-08-26 02:30:52.000000000 +0300
@@ -18,6 +18,5 @@
  #endif

  #define MAXHOSTNAMELEN	64	/* max length of hostname */
-#define COMMAND_LINE_SIZE 256

  #endif
diff -urNp linux-2.6.18-rc4-mm2/include/asm-i386/setup.h linux-2.6.18-rc4-mm2.new/include/asm-i386/setup.h
--- linux-2.6.18-rc4-mm2/include/asm-i386/setup.h	2006-08-25 16:10:56.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/include/asm-i386/setup.h	2006-08-26 02:30:52.000000000 +0300
@@ -15,7 +15,7 @@
  #define MAX_NONPAE_PFN	(1 << 20)

  #define PARAM_SIZE 4096
-#define COMMAND_LINE_SIZE 256
+#define COMMAND_LINE_SIZE 2048

  #define OLD_CL_MAGIC_ADDR	0x90020
  #define OLD_CL_MAGIC		0xA33F
diff -urNp linux-2.6.18-rc4-mm2/include/asm-ia64/setup.h linux-2.6.18-rc4-mm2.new/include/asm-ia64/setup.h
--- linux-2.6.18-rc4-mm2/include/asm-ia64/setup.h	2006-06-18 04:49:35.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/include/asm-ia64/setup.h	2006-08-26 02:30:52.000000000 +0300
@@ -1,6 +1,6 @@
  #ifndef __IA64_SETUP_H
  #define __IA64_SETUP_H

-#define COMMAND_LINE_SIZE	512
+#define COMMAND_LINE_SIZE	2048

  #endif
diff -urNp linux-2.6.18-rc4-mm2/include/asm-x86_64/setup.h linux-2.6.18-rc4-mm2.new/include/asm-x86_64/setup.h
--- linux-2.6.18-rc4-mm2/include/asm-x86_64/setup.h	2006-06-18 04:49:35.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/include/asm-x86_64/setup.h	2006-08-26 02:32:44.000000000 +0300
@@ -1,6 +1,6 @@
  #ifndef _x8664_SETUP_H
  #define _x8664_SETUP_H

-#define COMMAND_LINE_SIZE	256
+#define COMMAND_LINE_SIZE	2048

  #endif

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Andi Kleen]

Alon Bar-Lev <alon.barlev@gmail.com> writes:

> Extending the kernel parameters to a 2048 bytes for
> boot protocol >=2.02 of i386, ia64 and x86_64 architectures for
> linux-2.6.18-rc4-mm2.
> 
> Current implementation allows the kernel to receive up to
> 255 characters from the bootloader. In current environment,
> the command-line is used in order to specify many values,
> including suspend/resume, module arguments, splash, initramfs
> and more. 255 characters are not enough anymore.

The last time I tried this on x86-64 lilo on systems that used EDD broke.
EDD uses part of the bootup page too. So most likely it's not that simple.

And please don't shout your subjects.

-Andi

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Andi Kleen wrote:
> 
> The last time I tried this on x86-64 lilo on systems that used EDD broke.
> EDD uses part of the bootup page too. So most likely it's not that simple.
> 
> And please don't shout your subjects.
> 

On i386, the command line is never stored in the bootup page; only a 
pointer to it is.  The copying is done straight into the 
saved_command_line buffer in the kernel BSS (head.S lines 79-104).

x86-64 does the same thing, but in C code (head64.c lines 45-56.)  Thus, 
if you had a problem with LILO, I suspect the problem was inside LILO 
itself, and not a kernel issue.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Andi Kleen]

On Sunday 27 August 2006 20:50, H. Peter Anvin wrote:
> Andi Kleen wrote:
> > 
> > The last time I tried this on x86-64 lilo on systems that used EDD broke.
> > EDD uses part of the bootup page too. So most likely it's not that simple.
> > 
> > And please don't shout your subjects.
> > 
> 
> On i386, the command line is never stored in the bootup page; only a 
> pointer to it is.  The copying is done straight into the 
> saved_command_line buffer in the kernel BSS (head.S lines 79-104).
> 
> x86-64 does the same thing, but in C code (head64.c lines 45-56.)  Thus, 
> if you had a problem with LILO, I suspect the problem was inside LILO 
> itself, and not a kernel issue.

Just increasing that constant caused various lilo setups to not boot
anymore. I don't know who is actually to blame, just wanting to
point out that this "obvious" patch isn't actually that obvious.

-Andi

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Andi Kleen wrote:
> 
> Just increasing that constant caused various lilo setups to not boot
> anymore. I don't know who is actually to blame, just wanting to
> point out that this "obvious" patch isn't actually that obvious.
> 

How would that even be possible (unless you recompiled LILO with the new 
headers)?  There would be no difference in the memory image at the point 
LILO hands off to the kernel.

In order to reproduce this we need some details about your "various LILO 
setups", or this will remain as a source of cargo cult programming.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

On 8/27/06, Andi Kleen <ak@suse.de> wrote:
> Just increasing that constant caused various lilo setups to not boot
> anymore. I don't know who is actually to blame, just wanting to
> point out that this "obvious" patch isn't actually that obvious.
>
> -Andi
>

Hello,

lilo has its own COMMAND_LINE_SIZE constant.  It is not depended on
the kernel one.

lilo-22.7 lilo.h:
#define COMMAND_LINE_SIZE        256     /* CL_LENGTH */

lilo-22.7.2 lilo.h:
#define COMMAND_LINE_SIZE       512     /* CL_LENGTH */

So at worse case scenario it passes 256 bytes into the kernel
truncated with null terminated. Best case scenario it passes 512
bytes. Anyway... As long as you don't modify lilo, the kernel can
expect what-ever command-line length it wishes and truncate it to
match its own COMMAND_LINE_SIZE.

Please notice that we modify the kernel so it can accept long command
lines at boot protocol >= 2.02, but we don't modify  lilo behavior. So
lilo user will not be able to use the long command line size, until
lilo is modified to support it.

There is also a problem with grub, I've written a patch for it:
https://savannah.gnu.org/bugs/?func=detailitem&item_id=13606

Best Regards,
Alon Bar-Lev.

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Andi Kleen]

On Sunday 27 August 2006 21:32, H. Peter Anvin wrote:
> Andi Kleen wrote:
> > 
> > Just increasing that constant caused various lilo setups to not boot
> > anymore. I don't know who is actually to blame, just wanting to
> > point out that this "obvious" patch isn't actually that obvious.
> > 
> 
> How would that even be possible (unless you recompiled LILO with the new 
> headers)?  There would be no difference in the memory image at the point 
> LILO hands off to the kernel.

AFAIK the problem was that some EDD data got overwritten.

> 
> In order to reproduce this we need some details about your "various LILO 
> setups", or this will remain as a source of cargo cult programming.

You can search the mailing list archives, it's all in there if you don't
belive me.

-Andi

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Andi Kleen wrote:
> On Sunday 27 August 2006 21:32, H. Peter Anvin wrote:
>> Andi Kleen wrote:
>>> Just increasing that constant caused various lilo setups to not boot
>>> anymore. I don't know who is actually to blame, just wanting to
>>> point out that this "obvious" patch isn't actually that obvious.
>>>
>> How would that even be possible (unless you recompiled LILO with the new 
>> headers)?  There would be no difference in the memory image at the point 
>> LILO hands off to the kernel.
> 
> AFAIK the problem was that some EDD data got overwritten.
> 
>> In order to reproduce this we need some details about your "various LILO 
>> setups", or this will remain as a source of cargo cult programming.
> 
> You can search the mailing list archives, it's all in there if you don't
> belive me.
> 

Found the references.  This seems to imply that EDD overwrites the area 
used by LILO 22.6.1.  LILO 22.6.1 uses the new boot protocol, with the 
full pointer, and seems to obey the spec as far as I can read the code. 
  I'm going to try to run it in simulation and observe the failure that way.

However, something is still seriously out of joint.  The EDD data 
actually overlays the setup code, not the bootsect code, and thus there 
"shouldn't" be any way that this could interfere.  My best guess at this 
time is that either the EDD code or LILO uses memory it's not supposed 
to use, and the simulation should hopefully reveal that.

Sorry if I seem snarky on this, but if we can't get to the bottom of 
this we can't ever fix it.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[John Coffman]

LILO memory usage:

000600 - 001000 BIOS data check area.  Okay to overwrite.  LILO usage 
suppressed with command line "nobd" option.  There's also a config 
file option to suppress usage.

LILO typically loads at 9000:0000 up to the top of the EBDA.  Top of 
EBDA is determined by "int 12h."  Some BIOS's on add-in cards do not 
properly allocate the EBDA.  Use LILO Makefile option "BUG_SI_EBDA" 
to allocate extra EBDA for the BIOS.  If the BIOS data check area is 
created at boot time by LILO, then:

    >  lilo -T ebda

will tell you where LILO is loaded on your system.

--John


At 02:39 PM  Sunday 8/27/2006, H. Peter Anvin wrote:
>Andi Kleen wrote:
>>On Sunday 27 August 2006 21:32, H. Peter Anvin wrote:
>>>Andi Kleen wrote:
>>>>Just increasing that constant caused various lilo setups to not boot
>>>>anymore. I don't know who is actually to blame, just wanting to
>>>>point out that this "obvious" patch isn't actually that obvious.
>>>How would that even be possible (unless you recompiled LILO with 
>>>the new headers)?  There would be no difference in the memory 
>>>image at the point LILO hands off to the kernel.
>>AFAIK the problem was that some EDD data got overwritten.
>>
>>>In order to reproduce this we need some details about your 
>>>"various LILO setups", or this will remain as a source of cargo 
>>>cult programming.
>>You can search the mailing list archives, it's all in there if you don't
>>belive me.
>
>Found the references.  This seems to imply that EDD overwrites the 
>area used by LILO 22.6.1.  LILO 22.6.1 uses the new boot protocol, 
>with the full pointer, and seems to obey the spec as far as I can 
>read the code.  I'm going to try to run it in simulation and observe 
>the failure that way.
>
>However, something is still seriously out of joint.  The EDD data 
>actually overlays the setup code, not the bootsect code, and thus 
>there "shouldn't" be any way that this could interfere.  My best 
>guess at this time is that either the EDD code or LILO uses memory 
>it's not supposed to use, and the simulation should hopefully reveal that.
>
>Sorry if I seem snarky on this, but if we can't get to the bottom of 
>this we can't ever fix it.
>
>         -hpa


         PGP KeyID: 6781C9C8  (good until 31-Dec-2008)
         Keyserver at  ldap://keyserver.pgp.com  OR  http://pgp.mit.edu
         LILO links at http://freshmeat.net/projects/lilo
         and Help link at http://lilo.go.dyndns.org

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

H. Peter Anvin wrote:
> Found the references.  This seems to imply that EDD overwrites the area 
> used by LILO 22.6.1.  LILO 22.6.1 uses the new boot protocol, with the 
> full pointer, and seems to obey the spec as far as I can read the code. 
>  I'm going to try to run it in simulation and observe the failure that way.
> 
> However, something is still seriously out of joint.  The EDD data 
> actually overlays the setup code, not the bootsect code, and thus there 
> "shouldn't" be any way that this could interfere.  My best guess at this 
> time is that either the EDD code or LILO uses memory it's not supposed 
> to use, and the simulation should hopefully reveal that.
> 
> Sorry if I seem snarky on this, but if we can't get to the bottom of 
> this we can't ever fix it.
> 
>     -hpa
> 

I think I've found one problem... But I it should not be the major one.
The EDD code scans the command-line as fixed string.
What about something like the following?

Best Regards,
Alon Bar-Lev.

diff -urNp linux-2.6.18-rc4-mm2/arch/i386/boot/edd.S linux-2.6.18-rc4-mm2.new/arch/i386/boot/edd.S
--- linux-2.6.18-rc4-mm2/arch/i386/boot/edd.S   2006-06-18 04:49:35.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/arch/i386/boot/edd.S       2006-08-28 08:55:01.000000000 +0300
@@ -29,6 +29,8 @@
         movl    $(COMMAND_LINE_SIZE-7), %ecx
  # loop through kernel command line one byte at a time
  cl_loop:
+       cmpb    $0,(%si)
+       jz      done_cl
         cmpl    $EDD_CL_EQUALS, (%si)
         jz      found_edd_equals
         incl    %esi

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

Alon Bar-Lev wrote:
> H. Peter Anvin wrote:
>> Found the references.  This seems to imply that EDD overwrites the 
>> area used by LILO 22.6.1.  LILO 22.6.1 uses the new boot protocol, 
>> with the full pointer, and seems to obey the spec as far as I can read 
>> the code.  I'm going to try to run it in simulation and observe the 
>> failure that way.
>>
>> However, something is still seriously out of joint.  The EDD data 
>> actually overlays the setup code, not the bootsect code, and thus 
>> there "shouldn't" be any way that this could interfere.  My best guess 
>> at this time is that either the EDD code or LILO uses memory it's not 
>> supposed to use, and the simulation should hopefully reveal that.
>>
>> Sorry if I seem snarky on this, but if we can't get to the bottom of 
>> this we can't ever fix it.
>>
>>     -hpa
>>
> 
> I think I've found one problem... But I it should not be the major one.
> The EDD code scans the command-line as fixed string.
> What about something like the following?
> 
> Best Regards,
> Alon Bar-Lev.
> 
> diff -urNp linux-2.6.18-rc4-mm2/arch/i386/boot/edd.S 
> linux-2.6.18-rc4-mm2.new/arch/i386/boot/edd.S
> --- linux-2.6.18-rc4-mm2/arch/i386/boot/edd.S   2006-06-18 
> 04:49:35.000000000 +0300
> +++ linux-2.6.18-rc4-mm2.new/arch/i386/boot/edd.S       2006-08-28 
> 08:55:01.000000000 +0300
> @@ -29,6 +29,8 @@
>         movl    $(COMMAND_LINE_SIZE-7), %ecx
>  # loop through kernel command line one byte at a time
>  cl_loop:
> +       cmpb    $0,(%si)
> +       jz      done_cl
>         cmpl    $EDD_CL_EQUALS, (%si)
>         jz      found_edd_equals
>         incl    %esi
> 

Better patch.
I've noticed that this code sets esi but then reference using si... So fixed to
use esi (It worked so far since we are in low area... But I think using the same
register type is cleaner...)

Best Regards,
Alon Bar-Lev.

diff -urNp linux-2.6.18-rc4-mm2/arch/i386/boot/edd.S linux-2.6.18-rc4-mm2.new/arch/i386/boot/edd.S
--- linux-2.6.18-rc4-mm2/arch/i386/boot/edd.S   2006-06-18 04:49:35.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/arch/i386/boot/edd.S       2006-08-28 09:34:39.000000000 +0300
@@ -29,7 +29,9 @@
         movl    $(COMMAND_LINE_SIZE-7), %ecx
  # loop through kernel command line one byte at a time
  cl_loop:
-       cmpl    $EDD_CL_EQUALS, (%si)
+       cmpb    $0,(%esi)
+       jz      done_cl
+       cmpl    $EDD_CL_EQUALS, (%esi)
         jz      found_edd_equals
         incl    %esi
         loop    cl_loop
@@ -37,9 +39,9 @@ cl_loop:
  found_edd_equals:
  # only looking at first two characters after equals
         addl    $4, %esi
-       cmpw    $EDD_CL_OFF, (%si)      # edd=of
+       cmpw    $EDD_CL_OFF, (%esi)     # edd=of
         jz      do_edd_off
-       cmpw    $EDD_CL_SKIP, (%si)     # edd=sk
+       cmpw    $EDD_CL_SKIP, (%esi)    # edd=sk
         jz      do_edd_skipmbr
         jmp     done_cl
  do_edd_skipmbr:

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Alon Bar-Lev wrote:
> 
> Better patch.
> I've noticed that this code sets esi but then reference using si... So 
> fixed to
> use esi (It worked so far since we are in low area... But I think using 
> the same
> register type is cleaner...)
> 

Totally pointless since we're in 16-bit mode (as is the "incl %esi")... 
I guess it's "better" in the sense that if we run out of that we'll 
crash due to a segment overrun... maybe (some BIOSes leave us 
unknowningly in big real mode...)

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

On 8/28/06, H. Peter Anvin <hpa@zytor.com> wrote:
> Totally pointless since we're in 16-bit mode (as is the "incl %esi")...
> I guess it's "better" in the sense that if we run out of that we'll
> crash due to a segment overrun... maybe (some BIOSes leave us
> unknowningly in big real mode...)

So leave as is? Loading address into esi and reference as si?
Or modify the whole code to use 16 bits?

Best Regards,
Alon Bar-Lev.

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Alon Bar-Lev wrote:
> On 8/28/06, H. Peter Anvin <hpa@zytor.com> wrote:
>> Totally pointless since we're in 16-bit mode (as is the "incl %esi")...
>> I guess it's "better" in the sense that if we run out of that we'll
>> crash due to a segment overrun... maybe (some BIOSes leave us
>> unknowningly in big real mode...)
> 
> So leave as is? Loading address into esi and reference as si?
> Or modify the whole code to use 16 bits?
> 

Probably modifying the whole code to use 16 bits, unless there is a 
specific reason not to (Matt?)

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Matt Domsch]

On Mon, Aug 28, 2006 at 11:28:24AM -0700, H. Peter Anvin wrote:
> Alon Bar-Lev wrote:
> >On 8/28/06, H. Peter Anvin <hpa@zytor.com> wrote:
> >>Totally pointless since we're in 16-bit mode (as is the "incl %esi")...
> >>I guess it's "better" in the sense that if we run out of that we'll
> >>crash due to a segment overrun... maybe (some BIOSes leave us
> >>unknowningly in big real mode...)
> >
> >So leave as is? Loading address into esi and reference as si?
> >Or modify the whole code to use 16 bits?
> >
> 
> Probably modifying the whole code to use 16 bits, unless there is a 
> specific reason not to (Matt?)

No reason.  I was just trying to be careful, not leaving data in the
upper bits of those registers going uninitialized.  If we know they're
not being used ever, then it's not a problem.  But I don't think
that's the source of the command line size concern, is it?

Thanks,
Matt

-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Matt Domsch wrote:
> 
> No reason.  I was just trying to be careful, not leaving data in the
> upper bits of those registers going uninitialized.  If we know they're
> not being used ever, then it's not a problem.  But I don't think
> that's the source of the command line size concern, is it?
> 

No, it's treating the command line as a fixed buffer, as opposed to a 
null-terminated string.  This was always a bug, by the way.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

On 8/28/06, Matt Domsch <Matt_Domsch@dell.com> wrote:
> No reason.  I was just trying to be careful, not leaving data in the
> upper bits of those registers going uninitialized.  If we know they're
> not being used ever, then it's not a problem.  But I don't think
> that's the source of the command line size concern, is it?

Since the cmd_line_ptr is 32bit, we can load the lower 16bits into si,
ignoring the upper 16bits, or we can use esi for all references.
I think using esi for all references is cleaner...

Best Regards,
Alon Bar-Lev.

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Matt Domsch]

On Mon, Aug 28, 2006 at 12:00:37PM -0700, H. Peter Anvin wrote:
> Matt Domsch wrote:
> >
> >No reason.  I was just trying to be careful, not leaving data in the
> >upper bits of those registers going uninitialized.  If we know they're
> >not being used ever, then it's not a problem.  But I don't think
> >that's the source of the command line size concern, is it?
> >
> 
> No, it's treating the command line as a fixed buffer, as opposed to a 
> null-terminated string.  This was always a bug, by the way.

OK, I'll look at fixing that, and using %esi throughout.

Thanks,
Matt


-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

[-- Attachment #1: Type: text/plain, Size: 191 bytes --]

On 8/28/06, Matt Domsch <Matt_Domsch@dell.com> wrote:
> OK, I'll look at fixing that, and using %esi throughout.
>
> Thanks,
> Matt
>

Something as the attached?

Best Regards,
Alon Bar-Lev.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: edd-esi-null.diff --]
[-- Type: text/x-diff; name="edd-esi-null.diff", Size: 1057 bytes --]

--- linux-2.6.18-rc4-mm2/arch/i386/boot/edd.S	2006-06-18 04:49:35.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/arch/i386/boot/edd.S	2006-08-28 23:21:01.000000000 +0300
@@ -22,14 +22,16 @@
 # edd=of  disables EDD completely  (edd=off)
 # edd=sk  skips the MBR test    (edd=skipmbr)
 	pushl	%esi
-    	cmpl	$0, %cs:cmd_line_ptr
-	jz	done_cl
 	movl	%cs:(cmd_line_ptr), %esi
+	andl	%esi, %esi
+	jz	done_cl
 # ds:esi has the pointer to the command line now
-	movl	$(COMMAND_LINE_SIZE-7), %ecx
+	movw	$(COMMAND_LINE_SIZE-7), %cx
 # loop through kernel command line one byte at a time
 cl_loop:
-	cmpl	$EDD_CL_EQUALS, (%si)
+	cmpb	$0, (%esi)
+	jz	done_cl
+	cmpl	$EDD_CL_EQUALS, (%esi)
 	jz	found_edd_equals
 	incl	%esi
 	loop	cl_loop
@@ -37,9 +39,9 @@ cl_loop:
 found_edd_equals:
 # only looking at first two characters after equals
     	addl	$4, %esi
-	cmpw	$EDD_CL_OFF, (%si)	# edd=of
+	cmpw	$EDD_CL_OFF, (%esi)	# edd=of
 	jz	do_edd_off
-	cmpw	$EDD_CL_SKIP, (%si)	# edd=sk
+	cmpw	$EDD_CL_SKIP, (%esi)	# edd=sk
 	jz	do_edd_skipmbr
 	jmp	done_cl
 do_edd_skipmbr:

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Alon Bar-Lev wrote:
> On 8/28/06, Matt Domsch <Matt_Domsch@dell.com> wrote:
>> No reason.  I was just trying to be careful, not leaving data in the
>> upper bits of those registers going uninitialized.  If we know they're
>> not being used ever, then it's not a problem.  But I don't think
>> that's the source of the command line size concern, is it?
> 
> Since the cmd_line_ptr is 32bit, we can load the lower 16bits into si,
> ignoring the upper 16bits, or we can use esi for all references.
> I think using esi for all references is cleaner...
> 

Bullshit.  You're in 16 bit mode, and your segment limits are only 64K 
in size, so you HAVE to use a segment:offset type addressing:

Thus, you want to do something like this.

	movl	cmd_line_ptr, %esi
	movl	%esi, %eax
	shrl	$4, %eax
	mov	%ax, %es
	andw	$0xf, %si

... and then address it through es:si.  Anything else is total, utterly 
and completely wrong.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Matt Domsch wrote:
> On Mon, Aug 28, 2006 at 12:00:37PM -0700, H. Peter Anvin wrote:
>> Matt Domsch wrote:
>>> No reason.  I was just trying to be careful, not leaving data in the
>>> upper bits of those registers going uninitialized.  If we know they're
>>> not being used ever, then it's not a problem.  But I don't think
>>> that's the source of the command line size concern, is it?
>>>
>> No, it's treating the command line as a fixed buffer, as opposed to a 
>> null-terminated string.  This was always a bug, by the way.
> 
> OK, I'll look at fixing that, and using %esi throughout.
> 

NAK on that.  "Using %esi throughout" is a red herring, and just will 
produce worse code for no gain.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Matt Domsch wrote:
> On Mon, Aug 28, 2006 at 12:00:37PM -0700, H. Peter Anvin wrote:
>> Matt Domsch wrote:
>>> No reason.  I was just trying to be careful, not leaving data in the
>>> upper bits of those registers going uninitialized.  If we know they're
>>> not being used ever, then it's not a problem.  But I don't think
>>> that's the source of the command line size concern, is it?
>>>
>> No, it's treating the command line as a fixed buffer, as opposed to a 
>> null-terminated string.  This was always a bug, by the way.
> 
> OK, I'll look at fixing that, and using %esi throughout.
> 

There is a lot of weirdness in this code; it's broken in an enormous 
amount of ways (sorry, Matt).  This comment, for example:

	pushl	%esi
     	cmpl	$0, %cs:cmd_line_ptr
	jz	done_cl
	movl	%cs:(cmd_line_ptr), %esi
# ds:esi has the pointer to the command line now

... doesn't handle the old boot protocol, and doesn't at all deal with 
the fact that cmd_line_ptr is an absolute address, and not at all 
relative to SETUPSEG, which is the normal value for %ds at this point. 
For the old protocol, this is a 16-bit pointer which is relative to 
INITSEG (not SETUPSEG), but this code just completely ignores it.

I'll hack up a patch for this.

	-hpa

[PATCH] Fix the EDD code misparsing the command line

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: edd-cmdline-fix.path --]
[-- Type: text/plain, Size: 3570 bytes --]

The EDD code would scan the command line as a fixed array, without
taking account of either whitespace, null-termination, the old
command-line protocol, late overrides early, or the fact that the
command line may not be reachable from INITSEG.

This should fix those problems, and enable us to use a longer command
line.

Signed-off-by: H. Peter Anvin <hpa@zytor.com>


diff --git a/arch/i386/boot/edd.S b/arch/i386/boot/edd.S
index 4b84ea2..03712a0 100644
--- a/arch/i386/boot/edd.S
+++ b/arch/i386/boot/edd.S
@@ -15,42 +15,90 @@ #include <linux/edd.h>
 #include <asm/setup.h>
 
 #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
-	movb	$0, (EDD_MBR_SIG_NR_BUF)
-	movb	$0, (EDDNR)
 
-# Check the command line for two options:
+# It is assumed that %ds == INITSEG here
+	
+	movb	$0, EDD_MBR_SIG_NR_BUF
+	movb	$0, EDDNR
+
+# Check the command line for options:
 # edd=of  disables EDD completely  (edd=off)
 # edd=sk  skips the MBR test    (edd=skipmbr)
+# edd=on  re-enables EDD (edd=on)
+	
 	pushl	%esi
-    	cmpl	$0, %cs:cmd_line_ptr
-	jz	done_cl
+	movw	$edd_mbr_sig_start, %di	# Default to edd=on
+	
 	movl	%cs:(cmd_line_ptr), %esi
-# ds:esi has the pointer to the command line now
-	movl	$(COMMAND_LINE_SIZE-7), %ecx
+	andl	%esi, %esi
+	jz	old_cl			# Old boot protocol?
+
+# Convert to a real-mode pointer in fs:si
+	movl	%esi, %eax
+	shrl	$4, %eax
+	movw	%ax, %fs
+	andw	$0xf, %si
+	jmp	have_cl_pointer
+
+# Old-style boot protocol?
+old_cl:
+	push	%ds			# aka INITSEG
+	pop	%fs
+
+	cmpw	$0xa33f, (0x20)
+	jne	done_cl			# No command line at all?
+	movw	(0x22), %si		# Pointer relative to INITSEG
+
+# fs:si has the pointer to the command line now
+have_cl_pointer:
+	
 # loop through kernel command line one byte at a time
-cl_loop:
-	cmpl	$EDD_CL_EQUALS, (%si)
+cl_atspace:
+	movl	%fs:(%si), %eax
+	andb	%al, %al		# End of line?
+	jz	done_cl
+	cmpl	$EDD_CL_EQUALS, %eax
 	jz	found_edd_equals
-	incl	%esi
-	loop	cl_loop
-	jmp	done_cl
+	cmpb	$0x20, %al		# <= space consider whitespace
+	ja	cl_skipword
+	incw	%si
+	jnz	cl_atspace
+	jmp	done_cl			# Wraparound...
+
+cl_skipword:
+	movb	%fs:(%si), %al		# End of string?
+	andb	%al, %al
+	jz	done_cl
+	cmpb	$0x20, %al
+	jbe	cl_atspace
+	incw	%si
+	jnz	cl_skipword
+	jmp	done_cl			# Wraparound...
+	
 found_edd_equals:
 # only looking at first two characters after equals
-    	addl	$4, %esi
-	cmpw	$EDD_CL_OFF, (%si)	# edd=of
-	jz	do_edd_off
-	cmpw	$EDD_CL_SKIP, (%si)	# edd=sk
-	jz	do_edd_skipmbr
-	jmp	done_cl
+# late overrides early on the command line, so keep going after finding something
+	movw	%fs:4(%si), %ax
+	cmpw	$EDD_CL_OFF, %ax	# edd=of
+	je	do_edd_off
+	cmpw	$EDD_CL_SKIP, %ax	# edd=sk
+	je	do_edd_skipmbr
+	cmpw	$EDD_CL_ON, %ax		# edd=on
+	je	do_edd_on
+	jmp	cl_skipword
 do_edd_skipmbr:
-    	popl	%esi
-	jmp	edd_start
+	movw	$edd_start, %di
+	jmp	cl_skipword
 do_edd_off:
-	popl	%esi
-	jmp	edd_done
+	movw	$edd_done, %di
+	jmp	cl_skipword
+do_edd_on:
+	movw	$edd_mbr_sig_start, %di
+	jmp	cl_skipword
+	
 done_cl:
 	popl	%esi
-
+	jmpw	*%di
 
 # Read the first sector of each BIOS disk device and store the 4-byte signature
 edd_mbr_sig_start:
diff --git a/include/linux/edd.h b/include/linux/edd.h
index 162512b..b2b3e68 100644
--- a/include/linux/edd.h
+++ b/include/linux/edd.h
@@ -52,6 +52,7 @@ #define EDD_MBR_SIG_NR_BUF 0x1ea  /* add
 #define EDD_CL_EQUALS   0x3d646465     /* "edd=" */
 #define EDD_CL_OFF      0x666f         /* "of" for off  */
 #define EDD_CL_SKIP     0x6b73         /* "sk" for skipmbr */
+#define EDD_CL_ON       0x6e6f	       /* "on" for on */
 
 #ifndef __ASSEMBLY__
 

Re: [PATCH] Fix the EDD code misparsing the command line

[Petr Vandrovec]

H. Peter Anvin wrote:
> 
> 
> ------------------------------------------------------------------------
> 
> The EDD code would scan the command line as a fixed array, without
> taking account of either whitespace, null-termination, the old
> command-line protocol, late overrides early, or the fact that the
> command line may not be reachable from INITSEG.
> 
> This should fix those problems, and enable us to use a longer command
> line.
> 
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> 
> 
> diff --git a/arch/i386/boot/edd.S b/arch/i386/boot/edd.S
> index 4b84ea2..03712a0 100644
> --- a/arch/i386/boot/edd.S
> +++ b/arch/i386/boot/edd.S

> +	andl	%esi, %esi
> +	jz	old_cl			# Old boot protocol?
> +
> +# Convert to a real-mode pointer in fs:si
> +	movl	%esi, %eax
> +	shrl	$4, %eax
> +	movw	%ax, %fs
> +	andw	$0xf, %si
> +	jmp	have_cl_pointer
> +
> +# Old-style boot protocol?
> +old_cl:
> +	push	%ds			# aka INITSEG
> +	pop	%fs
> +
> +	cmpw	$0xa33f, (0x20)
> +	jne	done_cl			# No command line at all?
> +	movw	(0x22), %si		# Pointer relative to INITSEG

Perhaps you should convert ds:si to flat pointer and then this flat pointer to 
fs:si using method above, to avoid problems with dword access with si > 0xfffc 
or word access with si > 0xfffe ?

> +
> +# fs:si has the pointer to the command line now
> +have_cl_pointer:
> +	
>  # loop through kernel command line one byte at a time
> -cl_loop:
> -	cmpl	$EDD_CL_EQUALS, (%si)
> +cl_atspace:
> +	movl	%fs:(%si), %eax

This looks fine for new boot protocol, but what if old boot protocol puts 
command line so that its last byte is at INITSEG:0xffff ?  You get #GP here, 
then, although command line is correctly zero terminated and does not overflow 
segment.

> +	andb	%al, %al		# End of line?
> +	jz	done_cl
> +	cmpl	$EDD_CL_EQUALS, %eax
>  	jz	found_edd_equals
> -	incl	%esi
> -	loop	cl_loop
> -	jmp	done_cl
> +	cmpb	$0x20, %al		# <= space consider whitespace
> +	ja	cl_skipword
> +	incw	%si
> +	jnz	cl_atspace

You already died with #GP when si was 0xfffd or bigger above, so this ZF test is 
probably not quite needed.

> +	jmp	done_cl			# Wraparound...
> +
> +cl_skipword:
> +	movb	%fs:(%si), %al		# End of string?
> +	andb	%al, %al
> +	jz	done_cl
> +	cmpb	$0x20, %al
> +	jbe	cl_atspace
> +	incw	%si
> +	jnz	cl_skipword
> +	jmp	done_cl			# Wraparound...
> +	
>  found_edd_equals:
>  # only looking at first two characters after equals
> -    	addl	$4, %esi
> -	cmpw	$EDD_CL_OFF, (%si)	# edd=of
> -	jz	do_edd_off
> -	cmpw	$EDD_CL_SKIP, (%si)	# edd=sk
> -	jz	do_edd_skipmbr
> -	jmp	done_cl
> +# late overrides early on the command line, so keep going after finding something
> +	movw	%fs:4(%si), %ax

If si is 0xfffb here, bad things happen.  I know, things I've pointed out should 
not be problem in real world, and new code is definitely better than old one, 
but if you already have code to avoid endless loop if command line points to 
64KB array of 0xFF let's do that right, no?
						Thanks,
							Petr Vandrovec

Re: [PATCH] Fix the EDD code misparsing the command line

[H. Peter Anvin]

Petr Vandrovec wrote:
>> +
>> +# Old-style boot protocol?
>> +old_cl:
>> +    push    %ds            # aka INITSEG
>> +    pop    %fs
>> +
>> +    cmpw    $0xa33f, (0x20)
>> +    jne    done_cl            # No command line at all?
>> +    movw    (0x22), %si        # Pointer relative to INITSEG
> 
> Perhaps you should convert ds:si to flat pointer and then this flat 
> pointer to fs:si using method above, to avoid problems with dword access 
> with si > 0xfffc or word access with si > 0xfffe ?
> 
>> +
>> +# fs:si has the pointer to the command line now
>> +have_cl_pointer:
>> +   
>>  # loop through kernel command line one byte at a time
>> -cl_loop:
>> -    cmpl    $EDD_CL_EQUALS, (%si)
>> +cl_atspace:
>> +    movl    %fs:(%si), %eax
> 
> This looks fine for new boot protocol, but what if old boot protocol 
> puts command line so that its last byte is at INITSEG:0xffff ?  You get 
> #GP here, then, although command line is correctly zero terminated and 
> does not overflow segment.
> 

With the old protocol, the command line is supposed to fit inside the 
64K segment, so I don't think that's an issue.  Putting "Hail Mary" 
break at 0xfffd isn't a bad idea, though (especially since even if that 
is legitimate, we can't fit "edd=" into that one.)

> If si is 0xfffb here, bad things happen.  I know, things I've pointed 
> out should not be problem in real world, and new code is definitely 
> better than old one, but if you already have code to avoid endless loop 
> if command line points to 64KB array of 0xFF let's do that right, no?

Agreed.  I'll update the patch shortly.

	-hpa

Re: [PATCH] Fix the EDD code misparsing the command line (rev 2)

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: edd-cmdline-fix-2.patch --]
[-- Type: text/x-patch, Size: 3818 bytes --]

The EDD code would scan the command line as a fixed array, without
taking account of either whitespace, null-termination, the old
command-line protocol, late overrides early, or the fact that the
command line may not be reachable from INITSEG.

This should fix those problems, and enable us to use a longer command
line.

Signed-off-by: H. Peter Anvin <hpa@zytor.com>


diff --git a/arch/i386/boot/edd.S b/arch/i386/boot/edd.S
index 4b84ea2..5d52908 100644
--- a/arch/i386/boot/edd.S
+++ b/arch/i386/boot/edd.S
@@ -15,42 +15,95 @@ #include <linux/edd.h>
 #include <asm/setup.h>
 
 #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
+
+# It is assumed that %ds == INITSEG here
+	
 	movb	$0, (EDD_MBR_SIG_NR_BUF)
 	movb	$0, (EDDNR)
 
-# Check the command line for two options:
+# Check the command line for options:
 # edd=of  disables EDD completely  (edd=off)
 # edd=sk  skips the MBR test    (edd=skipmbr)
+# edd=on  re-enables EDD (edd=on)
+	
 	pushl	%esi
-    	cmpl	$0, %cs:cmd_line_ptr
-	jz	done_cl
+	movw	$edd_mbr_sig_start, %di	# Default to edd=on
+	
 	movl	%cs:(cmd_line_ptr), %esi
-# ds:esi has the pointer to the command line now
-	movl	$(COMMAND_LINE_SIZE-7), %ecx
-# loop through kernel command line one byte at a time
-cl_loop:
-	cmpl	$EDD_CL_EQUALS, (%si)
+	andl	%esi, %esi
+	jz	old_cl			# Old boot protocol?
+
+# Convert to a real-mode pointer in fs:si
+	movl	%esi, %eax
+	shrl	$4, %eax
+	movw	%ax, %fs
+	andw	$0xf, %si
+	jmp	have_cl_pointer
+
+# Old-style boot protocol?
+old_cl:
+	push	%ds			# aka INITSEG
+	pop	%fs
+
+	cmpw	$0xa33f, (0x20)
+	jne	done_cl			# No command line at all?
+	movw	(0x22), %si		# Pointer relative to INITSEG
+
+# fs:si has the pointer to the command line now
+have_cl_pointer:
+	
+# Loop through kernel command line one byte at a time.  Just in
+# case the loader is buggy and failed to null-terminate the command line
+# terminate if we get close enough to the end of the segment that we
+# cannot fit "edd=XX"...
+cl_atspace:
+	cmpw	$-5, %si		# Watch for segment wraparound
+	jae	done_cl
+	movl	%fs:(%si), %eax
+	andb	%al, %al		# End of line?
+	jz	done_cl
+	cmpl	$EDD_CL_EQUALS, %eax
 	jz	found_edd_equals
-	incl	%esi
-	loop	cl_loop
-	jmp	done_cl
+	cmpb	$0x20, %al		# <= space consider whitespace
+	ja	cl_skipword
+	incw	%si
+	jmp	cl_atspace
+
+cl_skipword:
+	cmpw	$-5, %si		# Watch for segment wraparound
+	jae	done_cl
+	movb	%fs:(%si), %al		# End of string?
+	andb	%al, %al
+	jz	done_cl
+	cmpb	$0x20, %al
+	jbe	cl_atspace
+	incw	%si
+	jmp	cl_skipword
+	
 found_edd_equals:
 # only looking at first two characters after equals
-    	addl	$4, %esi
-	cmpw	$EDD_CL_OFF, (%si)	# edd=of
-	jz	do_edd_off
-	cmpw	$EDD_CL_SKIP, (%si)	# edd=sk
-	jz	do_edd_skipmbr
-	jmp	done_cl
+# late overrides early on the command line, so keep going after finding something
+	movw	%fs:4(%si), %ax
+	cmpw	$EDD_CL_OFF, %ax	# edd=of
+	je	do_edd_off
+	cmpw	$EDD_CL_SKIP, %ax	# edd=sk
+	je	do_edd_skipmbr
+	cmpw	$EDD_CL_ON, %ax		# edd=on
+	je	do_edd_on
+	jmp	cl_skipword
 do_edd_skipmbr:
-    	popl	%esi
-	jmp	edd_start
+	movw	$edd_start, %di
+	jmp	cl_skipword
 do_edd_off:
-	popl	%esi
-	jmp	edd_done
+	movw	$edd_done, %di
+	jmp	cl_skipword
+do_edd_on:
+	movw	$edd_mbr_sig_start, %di
+	jmp	cl_skipword
+	
 done_cl:
 	popl	%esi
-
+	jmpw	*%di
 
 # Read the first sector of each BIOS disk device and store the 4-byte signature
 edd_mbr_sig_start:
diff --git a/include/linux/edd.h b/include/linux/edd.h
index 162512b..b2b3e68 100644
--- a/include/linux/edd.h
+++ b/include/linux/edd.h
@@ -52,6 +52,7 @@ #define EDD_MBR_SIG_NR_BUF 0x1ea  /* add
 #define EDD_CL_EQUALS   0x3d646465     /* "edd=" */
 #define EDD_CL_OFF      0x666f         /* "of" for off  */
 #define EDD_CL_SKIP     0x6b73         /* "sk" for skipmbr */
+#define EDD_CL_ON       0x6e6f	       /* "on" for on */
 
 #ifndef __ASSEMBLY__
 

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

Extending the kernel parameters to a 2048 bytes for
boot protocol >=2.02 of i386, ia64 and x86_64 architectures for
linux-2.6.18-rc4-mm2.

Current implementation allows the kernel to receive up to
255 characters from the bootloader. In current environment,
the command-line is used in order to specify many values,
including suspend/resume, module arguments, splash, initramfs
and more. 255 characters are not enough anymore.

EDD issue was fixed recently by H. Peter Anvin, please add this
to mm so more problems may be found.

Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>

---

diff -ruNp linux-2.6.18-rc4-mm2/include/asm-i386/param.h linux-2.6.18-rc4-mm2.new/include/asm-i386/param.h
--- linux-2.6.18-rc4-mm2/include/asm-i386/param.h	2006-08-25 16:10:56.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/include/asm-i386/param.h	2006-08-26 02:30:52.000000000 +0300
@@ -18,6 +18,5 @@
 #endif
 
 #define MAXHOSTNAMELEN	64	/* max length of hostname */
-#define COMMAND_LINE_SIZE 256
 
 #endif
diff -ruNp linux-2.6.18-rc4-mm2/include/asm-i386/setup.h linux-2.6.18-rc4-mm2.new/include/asm-i386/setup.h
--- linux-2.6.18-rc4-mm2/include/asm-i386/setup.h	2006-08-25 16:10:56.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/include/asm-i386/setup.h	2006-08-26 02:30:52.000000000 +0300
@@ -15,7 +15,7 @@
 #define MAX_NONPAE_PFN	(1 << 20)
 
 #define PARAM_SIZE 4096
-#define COMMAND_LINE_SIZE 256
+#define COMMAND_LINE_SIZE 2048
 
 #define OLD_CL_MAGIC_ADDR	0x90020
 #define OLD_CL_MAGIC		0xA33F
diff -ruNp linux-2.6.18-rc4-mm2/include/asm-ia64/setup.h linux-2.6.18-rc4-mm2.new/include/asm-ia64/setup.h
--- linux-2.6.18-rc4-mm2/include/asm-ia64/setup.h	2006-06-18 04:49:35.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/include/asm-ia64/setup.h	2006-08-26 02:30:52.000000000 +0300
@@ -1,6 +1,6 @@
 #ifndef __IA64_SETUP_H
 #define __IA64_SETUP_H
 
-#define COMMAND_LINE_SIZE	512
+#define COMMAND_LINE_SIZE	2048
 
 #endif
diff -ruNp linux-2.6.18-rc4-mm2/include/asm-x86_64/setup.h linux-2.6.18-rc4-mm2.new/include/asm-x86_64/setup.h
--- linux-2.6.18-rc4-mm2/include/asm-x86_64/setup.h	2006-06-18 04:49:35.000000000 +0300
+++ linux-2.6.18-rc4-mm2.new/include/asm-x86_64/setup.h	2006-08-26 02:32:44.000000000 +0300
@@ -1,6 +1,6 @@
 #ifndef _x8664_SETUP_H
 #define _x8664_SETUP_H
 
-#define COMMAND_LINE_SIZE	256
+#define COMMAND_LINE_SIZE	2048
 
 #endif

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Andi Kleen]

On Wednesday 30 August 2006 18:49, Alon Bar-Lev wrote:
> 
> Extending the kernel parameters to a 2048 bytes for
> boot protocol >=2.02 of i386, ia64 and x86_64 architectures for
> linux-2.6.18-rc4-mm2.
> 
> Current implementation allows the kernel to receive up to
> 255 characters from the bootloader. In current environment,
> the command-line is used in order to specify many values,
> including suspend/resume, module arguments, splash, initramfs
> and more. 255 characters are not enough anymore.
> 
> EDD issue was fixed recently by H. Peter Anvin, please add this
> to mm so more problems may be found.

IA64 booting is completely different. I don't think it should 
be in this patch. At least you would need to check with the IA64
maintainer first.

And the other thing is that this will cost memory. Either make
it dependend on !CONFIG_SMALL or fix the boot code to save the 
command line into a kmalloc'ed buffer of the right size and __init 
the original one

-Andi

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

On Wed, 30 Aug 2006 18:56:11 +0200
Andi Kleen <ak@suse.de> wrote:
> IA64 booting is completely different. I don't think it should 
> be in this patch. At least you would need to check with the IA64
> maintainer first.

OK... no problem.

> 
> And the other thing is that this will cost memory. Either make
> it dependend on !CONFIG_SMALL or fix the boot code to save the 
> command line into a kmalloc'ed buffer of the right size and __init 
> the original one

I don't mind doing either... Any preference for one of them? The
kmalloc approach seems nicer..

Best Regards,
Alon Bar-Lev.

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Andi Kleen]

On Wednesday 30 August 2006 19:06, Alon Bar-Lev wrote:

> > 
> > And the other thing is that this will cost memory. Either make
> > it dependend on !CONFIG_SMALL or fix the boot code to save the 
> > command line into a kmalloc'ed buffer of the right size and __init 
> > the original one
> 
> I don't mind doing either... Any preference for one of them? The
> kmalloc approach seems nicer..

kmalloc is better yes. You just have to do it after kmalloc is up
and running and make sure the users before reference the __init'ed version.
I suspect only /proc/cmdline will need the kmalloc version after booting, 
nobody else should look at the command line.

-Andi

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

On Wed, 30 Aug 2006 19:31:14 +0200
Andi Kleen <ak@suse.de> wrote:

> On Wednesday 30 August 2006 19:06, Alon Bar-Lev wrote:
> 
> > > 
> > > And the other thing is that this will cost memory. Either make
> > > it dependend on !CONFIG_SMALL or fix the boot code to save the 
> > > command line into a kmalloc'ed buffer of the right size and
> > > __init the original one
> > 
> > I don't mind doing either... Any preference for one of them? The
> > kmalloc approach seems nicer..
> 
> kmalloc is better yes. You just have to do it after kmalloc is up
> and running and make sure the users before reference the __init'ed
> version. I suspect only /proc/cmdline will need the kmalloc version
> after booting, nobody else should look at the command line.
> 
> -Andi

This is not entirely true...
All architectures sets saved_command_line variable...
So I can add __init to the saved_command_line and
copy its contents into kmalloced persistence_command_line at
main.c.

Then the following files should be modified to use the new kmalloced variable:

./drivers/sbus/char/openprom.c: char *buf = saved_command_line;
./fs/proc/kcore.c:      strncpy(prpsinfo.pr_psargs, saved_command_line, ELF_PRARGSZ);
./fs/proc/proc_misc.c:  len = sprintf(page, "%s\n", saved_command_line);

Have I got it right?

Best Regards,
Alon Bar-Lev.

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Alon Bar-Lev wrote:
> On Wed, 30 Aug 2006 18:56:11 +0200
> Andi Kleen <ak@suse.de> wrote:
>> IA64 booting is completely different. I don't think it should 
>> be in this patch. At least you would need to check with the IA64
>> maintainer first.
> 
> OK... no problem.
> 
>> And the other thing is that this will cost memory. Either make
>> it dependend on !CONFIG_SMALL or fix the boot code to save the 
>> command line into a kmalloc'ed buffer of the right size and __init 
>> the original one
> 
> I don't mind doing either... Any preference for one of them? The
> kmalloc approach seems nicer..
> 

The kmalloc approach seems to be The Right Thing.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Alon Bar-Lev wrote:
> 
> This is not entirely true...
> All architectures sets saved_command_line variable...
> So I can add __init to the saved_command_line and
> copy its contents into kmalloced persistence_command_line at
> main.c.
> 

My opinion is that you should change saved_command_line (which already 
implies a copy) to be the kmalloc'd version and call the fixed-sized 
buffer something else.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Andi Kleen]

On Wednesday 30 August 2006 20:59, H. Peter Anvin wrote:
> Alon Bar-Lev wrote:
> > 
> > This is not entirely true...
> > All architectures sets saved_command_line variable...
> > So I can add __init to the saved_command_line and
> > copy its contents into kmalloced persistence_command_line at
> > main.c.
> > 
> 
> My opinion is that you should change saved_command_line (which already 
> implies a copy) to be the kmalloc'd version and call the fixed-sized 
> buffer something else.

It might be safer to rename everything. Then all users could be caught
and audited. This would ensure saved_command_line is not accessed
before the kmalloc'ed copy exists.

Disadvantage: more architectures to change.

-Andi

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Andi Kleen wrote:
> On Wednesday 30 August 2006 20:59, H. Peter Anvin wrote:
>> Alon Bar-Lev wrote:
>>> This is not entirely true...
>>> All architectures sets saved_command_line variable...
>>> So I can add __init to the saved_command_line and
>>> copy its contents into kmalloced persistence_command_line at
>>> main.c.
>>>
>> My opinion is that you should change saved_command_line (which already 
>> implies a copy) to be the kmalloc'd version and call the fixed-sized 
>> buffer something else.
> 
> It might be safer to rename everything. Then all users could be caught
> and audited. This would ensure saved_command_line is not accessed
> before the kmalloc'ed copy exists.
> 
> Disadvantage: more architectures to change.
> 

That would definitely be the safest option, and probably is the way to go.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Alon Bar-Lev]

On Wed, 30 Aug 2006 11:59:40 -0700
"H. Peter Anvin" <hpa@zytor.com> wrote:

> Alon Bar-Lev wrote:
> > 
> > This is not entirely true...
> > All architectures sets saved_command_line variable...
> > So I can add __init to the saved_command_line and
> > copy its contents into kmalloced persistence_command_line at
> > main.c.
> > 
> 
> My opinion is that you should change saved_command_line (which
> already implies a copy) to be the kmalloc'd version and call the
> fixed-sized buffer something else.
> 
> 	-hpa

Changing saved_command_line is a modification to all
architectures... They all modify this variable...
So, should I submit a patch to all architectures? How can I test this?

Also for i386 the code is assembly... So I can modify the code to write
into a __init buffer and then kmalloc in setup.c.

Please instruct me how to proceed...

Best Regards,
Alon Bar-Lev.

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Alon Bar-Lev wrote:
>
> Changing saved_command_line is a modification to all
> architectures... They all modify this variable...
> So, should I submit a patch to all architectures? How can I test this?
> 

Submit a patch set, with the common changes in one patch and the 
architecture-specific bits broken out per architecture; that way the 
individual arch maintainers can look at their piece.  Since it's a 
simple variable rename, it shouldn't be a big deal.

> Also for i386 the code is assembly... So I can modify the code to write
> into a __init buffer and then kmalloc in setup.c.

Don't do that.  Just change the name of the buffer in head.S.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[H. Peter Anvin]

Bodo Eggert wrote:
> Andi Kleen <ak@suse.de> wrote:
>> On Wednesday 30 August 2006 20:59, H. Peter Anvin wrote:
>>> Alon Bar-Lev wrote:
> 
>>>> This is not entirely true...
>>>> All architectures sets saved_command_line variable...
>>>> So I can add __init to the saved_command_line and
>>>> copy its contents into kmalloced persistence_command_line at
>>>> main.c.
>>>>
>>> My opinion is that you should change saved_command_line (which already
>>> implies a copy) to be the kmalloc'd version and call the fixed-sized
>>> buffer something else.
>> It might be safer to rename everything. Then all users could be caught
>> and audited. This would ensure saved_command_line is not accessed
>> before the kmalloc'ed copy exists.
> 
> If you set the new *saved_cmdline=saved_cmdline_init, the users don't need
> to be adjusted at all, and you won't have trouble with code that may be
> run before or after kmallocking (if it exists).

True for C code, but not for assembly.

	-hpa

Re: [PATCH] THE LINUX/I386 BOOT PROTOCOL - Breaking the 256 limit (ping)

[Bodo Eggert]

Andi Kleen <ak@suse.de> wrote:
> On Wednesday 30 August 2006 20:59, H. Peter Anvin wrote:
>> Alon Bar-Lev wrote:

>> > This is not entirely true...
>> > All architectures sets saved_command_line variable...
>> > So I can add __init to the saved_command_line and
>> > copy its contents into kmalloced persistence_command_line at
>> > main.c.
>> > 
>> 
>> My opinion is that you should change saved_command_line (which already
>> implies a copy) to be the kmalloc'd version and call the fixed-sized
>> buffer something else.
> 
> It might be safer to rename everything. Then all users could be caught
> and audited. This would ensure saved_command_line is not accessed
> before the kmalloc'ed copy exists.

If you set the new *saved_cmdline=saved_cmdline_init, the users don't need
to be adjusted at all, and you won't have trouble with code that may be
run before or after kmallocking (if it exists).
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

http://david.woodhou.se/why-not-spf.html