[PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[Dave Kleikamp]

sparc64 defines COMMAND_LINE_SIZE to be 2048, but the boot string is
hard-coded to be 256 bytes long.

based on a patch by Bob Picco

Signed-off-by: Dave Kleikamp <***@oracle.com>
Cc: Bob Picco <***@oracle.com>
Cc: David S. Miller <***@davemloft.net>
Cc: ***@vger.kernel.org
---
arch/sparc/prom/bootstr_64.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/sparc/prom/bootstr_64.c b/arch/sparc/prom/bootstr_64.c
index ab9ccc6..4b9d73c 100644
--- a/arch/sparc/prom/bootstr_64.c
+++ b/arch/sparc/prom/bootstr_64.c
@@ -8,19 +8,19 @@
#include <linux/string.h>
#include <linux/init.h>
#include <asm/oplib.h>
+#include <asm/setup.h>

/* WARNING: The boot loader knows that these next three variables come one right
* after another in the .data section. Do not move this stuff into
* the .bss section or it will break things.
*/

-#define BARG_LEN 256
struct {
int bootstr_len;
int bootstr_valid;
- char bootstr_buf[BARG_LEN];
+ char bootstr_buf[COMMAND_LINE_SIZE];
} bootstr_info = {
- .bootstr_len = BARG_LEN,
+ .bootstr_len = COMMAND_LINE_SIZE,
#ifdef CONFIG_CMDLINE
.bootstr_valid = 1,
.bootstr_buf = CONFIG_CMDLINE,
@@ -34,7 +34,7 @@ prom_getbootargs(void)
if (bootstr_info.bootstr_valid)
return bootstr_info.bootstr_buf;
prom_getstring(prom_chosen_node, "bootargs",
- bootstr_info.bootstr_buf, BARG_LEN);
+ bootstr_info.bootstr_buf, COMMAND_LINE_SIZE);
bootstr_info.bootstr_valid = 1;
return bootstr_info.bootstr_buf;
}

[David Miller]

From: Dave Kleikamp <***@oracle.com>
Date: Mon, 06 Oct 2014 11:06:27 -0500

Post by Dave Kleikamp
sparc64 defines COMMAND_LINE_SIZE to be 2048, but the boot string is
hard-coded to be 256 bytes long.
based on a patch by Bob Picco

I think we might be limited by the size of barg_out[] in the SILO
bootloader, which is 1024 bytes.

SILO massages the command line into barg_out[] then does:

if (architecture == sun4u)
kernel_params = (char *)((hdrs->bootstr_info_ptr_low - 0x400000) +
(image_base - 0x4000));
...
if (kernel_params) {
extern char barg_out[];
int len = *(unsigned int *)kernel_params;

strncpy (kernel_params + 8, barg_out, len);
kernel_params [8 + len - 1] = 0;
*(unsigned int *)(kernel_params + 4) = 1;
}

So it blindly copies however many bytes you tell it to in that
bootstr_info area :-/

So I'm happy to apply a patch that increases the size to 1024,
whilst adding a comment explaining the SILO limitation.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dave Kleikamp]

Post by David Miller
Date: Mon, 06 Oct 2014 11:06:27 -0500

Post by Dave Kleikamp
sparc64 defines COMMAND_LINE_SIZE to be 2048, but the boot string is
hard-coded to be 256 bytes long.
based on a patch by Bob Picco

I think we might be limited by the size of barg_out[] in the SILO
bootloader, which is 1024 bytes.
if (architecture == sun4u)
kernel_params = (char *)((hdrs->bootstr_info_ptr_low - 0x400000) +
(image_base - 0x4000));
...
if (kernel_params) {
extern char barg_out[];
int len = *(unsigned int *)kernel_params;
strncpy (kernel_params + 8, barg_out, len);
kernel_params [8 + len - 1] = 0;
*(unsigned int *)(kernel_params + 4) = 1;
}
So it blindly copies however many bytes you tell it to in that
bootstr_info area :-/
So I'm happy to apply a patch that increases the size to 1024,
whilst adding a comment explaining the SILO limitation.

That makes sense. 1024 is much better than 256. I'll send a new patch.

Thanks,
Dave
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dave Kleikamp]

This is the longest boot string that silo supports.

Signed-off-by: Dave Kleikamp <***@oracle.com>
Cc: Bob Picco <***@oracle.com>
Cc: David S. Miller <***@davemloft.net>
Cc: ***@vger.kernel.org
---
arch/sparc/prom/bootstr_64.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/sparc/prom/bootstr_64.c b/arch/sparc/prom/bootstr_64.c
index ab9ccc6..4dac9f7 100644
--- a/arch/sparc/prom/bootstr_64.c
+++ b/arch/sparc/prom/bootstr_64.c
@@ -14,7 +14,11 @@
* the .bss section or it will break things.
*/

-#define BARG_LEN 256
+/*
+ * We limit BARG_LEN to 1024 rather than use COMMAND_LINE_SIZE because silo
+ * blindly copies however many bytes we tell it from a 1024-byte array.
+ */
+#define BARG_LEN 1024
struct {
int bootstr_len;
int bootstr_valid;

[David Miller]

From: Dave Kleikamp <***@oracle.com>
Date: Tue, 07 Oct 2014 08:12:37 -0500

Post by Dave Kleikamp
This is the longest boot string that silo supports.

Ok, given how the rest of this thread went, the comment needs to
be adjusted to match reality which is something like:

/* We limit BARG_LEN to 1024 because this is the size of the
* 'barg_out' command line buffer in the SILO bootloader. And
* it trusts bootstr_len over that buffer's size.
*/

Right?
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dave Kleikamp]

Post by David Miller
Date: Tue, 07 Oct 2014 08:12:37 -0500

Post by Dave Kleikamp
This is the longest boot string that silo supports.

Ok, given how the rest of this thread went, the comment needs to
/* We limit BARG_LEN to 1024 because this is the size of the
* 'barg_out' command line buffer in the SILO bootloader. And
* it trusts bootstr_len over that buffer's size.
*/
Right?

I'd leave off the last sentence:

/* We limit BARG_LEN to 1024 because this is the size of the
* 'barg_out' command line buffer in the SILO bootloader.
*/
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[David Miller]

From: Dave Kleikamp <***@oracle.com>
Date: Tue, 07 Oct 2014 14:29:25 -0500

Post by David Miller
/* We limit BARG_LEN to 1024 because this is the size of the
* 'barg_out' command line buffer in the SILO bootloader.
*/

Works for me.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dave Kleikamp]

Even though I just sent a new patch...

Post by David Miller
Date: Mon, 06 Oct 2014 11:06:27 -0500

Post by Dave Kleikamp
sparc64 defines COMMAND_LINE_SIZE to be 2048, but the boot string is
hard-coded to be 256 bytes long.
based on a patch by Bob Picco

I think we might be limited by the size of barg_out[] in the SILO
bootloader, which is 1024 bytes.
if (architecture == sun4u)
kernel_params = (char *)((hdrs->bootstr_info_ptr_low - 0x400000) +
(image_base - 0x4000));
...
if (kernel_params) {
extern char barg_out[];
int len = *(unsigned int *)kernel_params;
strncpy (kernel_params + 8, barg_out, len);
kernel_params [8 + len - 1] = 0;
*(unsigned int *)(kernel_params + 4) = 1;
}
So it blindly copies however many bytes you tell it to in that
bootstr_info area :-/

strncpy will still stop at the first null character, so the only way it
will copy more than 1024 bytes would be if it has already overflowed
barg_out in constructing the command line. Writing the final null
terminator into kernel_params is not a problem.

Post by David Miller
So I'm happy to apply a patch that increases the size to 1024,
whilst adding a comment explaining the SILO limitation.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Kjetil Oftedal]

Post by Dave Kleikamp
Even though I just sent a new patch...

Post by David Miller
Date: Mon, 06 Oct 2014 11:06:27 -0500

Post by Dave Kleikamp
sparc64 defines COMMAND_LINE_SIZE to be 2048, but the boot string is
hard-coded to be 256 bytes long.
based on a patch by Bob Picco

I think we might be limited by the size of barg_out[] in the SILO
bootloader, which is 1024 bytes.
if (architecture == sun4u)
kernel_params = (char *)((hdrs->bootstr_info_ptr_low - 0x400000) +
(image_base - 0x4000));
...
if (kernel_params) {
extern char barg_out[];
int len = *(unsigned int *)kernel_params;
strncpy (kernel_params + 8, barg_out, len);
kernel_params [8 + len - 1] = 0;
*(unsigned int *)(kernel_params + 4) = 1;
}
So it blindly copies however many bytes you tell it to in that
bootstr_info area :-/

strncpy will still stop at the first null character, so the only way it
will copy more than 1024 bytes would be if it has already overflowed
barg_out in constructing the command line. Writing the final null
terminator into kernel_params is not a problem.

Just a note from the strncpy manpage:
"If the length of src is less than n, strncpy() writes additional null
bytes to dest to ensure that a total of n bytes are written."

So strncpy always writes len bytes, regardless of the presence of a null char.
(Unless silo strncpy is different ?)
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dave Kleikamp]

Post by Kjetil Oftedal

Post by Dave Kleikamp
Even though I just sent a new patch...

Post by David Miller
Date: Mon, 06 Oct 2014 11:06:27 -0500

Post by Dave Kleikamp
sparc64 defines COMMAND_LINE_SIZE to be 2048, but the boot string is
hard-coded to be 256 bytes long.
based on a patch by Bob Picco

I think we might be limited by the size of barg_out[] in the SILO
bootloader, which is 1024 bytes.
if (architecture == sun4u)
kernel_params = (char *)((hdrs->bootstr_info_ptr_low - 0x400000) +
(image_base - 0x4000));
...
if (kernel_params) {
extern char barg_out[];
int len = *(unsigned int *)kernel_params;
strncpy (kernel_params + 8, barg_out, len);
kernel_params [8 + len - 1] = 0;
*(unsigned int *)(kernel_params + 4) = 1;
}
So it blindly copies however many bytes you tell it to in that
bootstr_info area :-/

strncpy will still stop at the first null character, so the only way it
will copy more than 1024 bytes would be if it has already overflowed
barg_out in constructing the command line. Writing the final null
terminator into kernel_params is not a problem.

"If the length of src is less than n, strncpy() writes additional null
bytes to dest to ensure that a total of n bytes are written."
So strncpy always writes len bytes, regardless of the presence of a null char.
(Unless silo strncpy is different ?)

I never realized that, but that's really not a problem since the
destination will have the space. The original patch shouldn't be
problematic, although it may make the string longer than it practically
needs to be. I'll let davem make the call which version he likes better.
Maybe the comment in the second patch can be improved.

Dave
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[David Miller]

From: Dave Kleikamp <***@oracle.com>
Date: Tue, 07 Oct 2014 13:49:37 -0500

Post by Dave Kleikamp
I never realized that, but that's really not a problem since the
destination will have the space. The original patch shouldn't be
problematic, although it may make the string longer than it practically
needs to be. I'll let davem make the call which version he likes better.
Maybe the comment in the second patch can be improved.

If you make the boot string 2048 bytes, and the firmware provides
a string longer than 1024 bytes to SILO, it will read past the end
of barg_buf.

So I think we still need to limit it to 1024.

In the long term, we can 'fix' SILO in some backwards compatible way.
I think the thing to do is keep putting 1024 into bootstr_len and then
bump the HdrS version (currently 0x0301) and make that new version
mean "ignore bootstr_len, command line buffer is 2048 bytes".
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dave Kleikamp]

Post by David Miller
Date: Tue, 07 Oct 2014 13:49:37 -0500

Post by Dave Kleikamp
I never realized that, but that's really not a problem since the
destination will have the space. The original patch shouldn't be
problematic, although it may make the string longer than it practically
needs to be. I'll let davem make the call which version he likes better.
Maybe the comment in the second patch can be improved.

If you make the boot string 2048 bytes, and the firmware provides
a string longer than 1024 bytes to SILO, it will read past the end
of barg_buf.

Isn't it already too late by the time SILO looks at the length in
kernel_params? silo_set_bootargs() has already been called, so if
barg_buf is too small, SILO has already written past the end of it
(unless I'm missing some other bounds checking).

Post by David Miller
So I think we still need to limit it to 1024.

I hope we never have a need for anything longer.

Post by David Miller
In the long term, we can 'fix' SILO in some backwards compatible way.
I think the thing to do is keep putting 1024 into bootstr_len and then
bump the HdrS version (currently 0x0301) and make that new version
mean "ignore bootstr_len, command line buffer is 2048 bytes".

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[David Miller]

From: Dave Kleikamp <***@oracle.com>
Date: Tue, 07 Oct 2014 14:19:19 -0500

Post by Dave Kleikamp

Post by David Miller
Date: Tue, 07 Oct 2014 13:49:37 -0500

Post by Dave Kleikamp
I never realized that, but that's really not a problem since the
destination will have the space. The original patch shouldn't be
problematic, although it may make the string longer than it practically
needs to be. I'll let davem make the call which version he likes better.
Maybe the comment in the second patch can be improved.

If you make the boot string 2048 bytes, and the firmware provides
a string longer than 1024 bytes to SILO, it will read past the end
of barg_buf.

Isn't it already too late by the time SILO looks at the length in
kernel_params? silo_set_bootargs() has already been called, so if
barg_buf is too small, SILO has already written past the end of it
(unless I'm missing some other bounds checking).

The more I look at it, we have a built in limitation of 512 bytes
actually.

Actually, for sparc64 OF (ie. P1275) it is careful to only bring
in 1024 bytes to barg_buf:

if (!full)
q = barg_buf;
else {
iter = prom_getproperty (prom_chosen, "bootpath", barg_buf, 510);
if (iter != -1)
if (iter && !barg_buf [iter - 1])
q = barg_buf + iter - 1;
else
q = barg_buf + iter;
else
q = barg_buf;
}
iter = prom_getproperty (prom_chosen, "bootargs", full ? q + 1 : q, 512);

Post by Dave Kleikamp

Post by David Miller
So I think we still need to limit it to 1024.

I hope we never have a need for anything longer.

Me too, let's just use 1024.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[David Miller]

From: Kjetil Oftedal <***@gmail.com>
Date: Tue, 7 Oct 2014 20:38:57 +0200

Post by Kjetil Oftedal
"If the length of src is less than n, strncpy() writes additional null
bytes to dest to ensure that a total of n bytes are written."
So strncpy always writes len bytes, regardless of the presence of a null char.
(Unless silo strncpy is different ?)

Let's take a look.

SILO's doesn't behave that way, it stops at the first NULL character.

char * strncpy(char *dest, const char *src, size_t count)
{
char *tmp = dest;

while (count-- && (*dest++ = *src++) != '\0')
/* nothing */;

return tmp;
}
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html