sparc64: use COMMAND_LINE_SIZE for boot string

PATCH In review Dave Kleikamp 3 participants 10 messages Oct 6, 2014 – Oct 7, 2014 SPARC + UltraSPARC (sparc/sparc64)

lore

Share & tools

lore

Dave Kleikamp

author

2014-10-06 16:06 UTC #0 lore raw

PATCH 1/1 sparc64: use COMMAND_LINE_SIZE for boot string

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 <[email protected]>

Cc Bob Picco <[email protected]>

Cc David S. Miller <[email protected]>

Cc [email protected]

---
 arch/sparc/prom/bootstr_64.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/sparc/prom/bootstr_64.c +4 -4

index ab9ccc6..4b9d73c 100644
@@ -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;
 }

Signature · 1 line

2.1.2

David Miller

maintainer

2014-10-07 04:23 UTC #1 lore raw

From: Dave Kleikamp <[email protected]>
Date: Mon, 06 Oct 2014 11:06:27 -0500

Quoted 6 lines

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 <[email protected]>

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.

Dave Kleikamp

author

2014-10-07 12:44 UTC #2 lore raw

In reply to Dave Kleikamp

Quoted 28 lines · David Miller

From: Dave Kleikamp <[email protected]>
Date: Mon, 06 Oct 2014 11:06:27 -0500
Quoted 6 lines
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 <[email protected]>
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;
	}

Quoted 5 lines

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

Dave Kleikamp

author

2014-10-07 13:23 UTC #3 lore raw

In reply to Dave Kleikamp

Even though I just sent a new patch...

Quoted 28 lines · David Miller

From: Dave Kleikamp <[email protected]>
Date: Mon, 06 Oct 2014 11:06:27 -0500
Quoted 6 lines
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 <[email protected]>
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;
	}

Quoted 2 lines

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.

Quoted 7 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kjetil Oftedal

2014-10-07 18:38 UTC #4 lore raw

In reply to Dave Kleikamp

Quoted 31 lines · Dave Kleikamp

Even though I just sent a new patch...
Quoted 28 lines · David Miller
From: Dave Kleikamp <[email protected]>
Date: Mon, 06 Oct 2014 11:06:27 -0500
Quoted 6 lines
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 <[email protected]>
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;
	}

Quoted 8 lines

Quoted 2 lines
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 ?)

Dave Kleikamp

author

2014-10-07 18:49 UTC #5 lore raw

In reply to Dave Kleikamp

Quoted 41 lines · Kjetil Oftedal

Quoted 31 lines · Dave Kleikamp
Even though I just sent a new patch...
Quoted 28 lines · David Miller
From: Dave Kleikamp <[email protected]>
Date: Mon, 06 Oct 2014 11:06:27 -0500
Quoted 6 lines
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 <[email protected]>
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;
	}
Quoted 8 lines
Quoted 2 lines
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.

Quoted 6 lines

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 ?)

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

David Miller

maintainer

2014-10-07 18:54 UTC #6 lore raw

In reply to Dave Kleikamp

From: Kjetil Oftedal <[email protected]>
Date: Tue, 7 Oct 2014 20:38:57 +0200

Quoted 6 lines

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 ?)

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;
}

David Miller

maintainer

2014-10-07 19:03 UTC #7 lore raw

In reply to Dave Kleikamp

From: Dave Kleikamp <[email protected]>
Date: Tue, 07 Oct 2014 13:49:37 -0500

Quoted 5 lines

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".

Dave Kleikamp

author

2014-10-07 19:19 UTC #8 lore raw

In reply to Dave Kleikamp

Quoted 9 lines · David Miller

From: Dave Kleikamp <[email protected]>
Date: Tue, 07 Oct 2014 13:49:37 -0500
Quoted 5 lines
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.

Quoted 3 lines

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).

Quoted 1 line

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

I hope we never have a need for anything longer.

Quoted 4 lines

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".

David Miller

maintainer

2014-10-07 19:23 UTC #9 lore raw

In reply to Dave Kleikamp

From: Dave Kleikamp <[email protected]>
Date: Tue, 07 Oct 2014 14:19:19 -0500

Quoted 10 lines · David Miller

Quoted 9 lines
From: Dave Kleikamp <[email protected]>
Date: Tue, 07 Oct 2014 13:49:37 -0500
Quoted 5 lines
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.

Quoted 8 lines

Quoted 3 lines
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);

Quoted 3 lines

Quoted 1 line
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.