[PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[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 <dave.kleikamp@oracle.com>
Cc: Bob Picco <bob.picco@oracle.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: sparclinux@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;
 }
-- 
2.1.2

Re: [PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[David Miller]

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

> 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 <dave.kleikamp@oracle.com>

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.

Re: [PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[Dave Kleikamp]

uOn 10/06/2014 11:23 PM, David Miller wrote:
> From: Dave Kleikamp <dave.kleikamp@oracle.com>
> Date: Mon, 06 Oct 2014 11:06:27 -0500
> 
>> 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 <dave.kleikamp@oracle.com>
> 
> 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.

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

Thanks,
Dave

Re: [PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[Dave Kleikamp]

Even though I just sent a new patch...

On 10/06/2014 11:23 PM, David Miller wrote:
> From: Dave Kleikamp <dave.kleikamp@oracle.com>
> Date: Mon, 06 Oct 2014 11:06:27 -0500
> 
>> 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 <dave.kleikamp@oracle.com>
> 
> 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 :-/

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.

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

Re: [PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[Kjetil Oftedal]

On 07/10/2014, Dave Kleikamp <dave.kleikamp@oracle.com> wrote:
> Even though I just sent a new patch...
>
> On 10/06/2014 11:23 PM, David Miller wrote:
>> From: Dave Kleikamp <dave.kleikamp@oracle.com>
>> Date: Mon, 06 Oct 2014 11:06:27 -0500
>>
>>> 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 <dave.kleikamp@oracle.com>
>>
>> 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 :-/
>
> 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 ?)

Re: [PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[Dave Kleikamp]

On 10/07/2014 01:38 PM, Kjetil Oftedal wrote:
> On 07/10/2014, Dave Kleikamp <dave.kleikamp@oracle.com> wrote:
>> Even though I just sent a new patch...
>>
>> On 10/06/2014 11:23 PM, David Miller wrote:
>>> From: Dave Kleikamp <dave.kleikamp@oracle.com>
>>> Date: Mon, 06 Oct 2014 11:06:27 -0500
>>>
>>>> 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 <dave.kleikamp@oracle.com>
>>>
>>> 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 :-/
>>
>> 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 ?)

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

Re: [PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[David Miller]

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

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

Re: [PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[David Miller]

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

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

Re: [PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[Dave Kleikamp]

On 10/07/2014 02:03 PM, David Miller wrote:
> From: Dave Kleikamp <dave.kleikamp@oracle.com>
> Date: Tue, 07 Oct 2014 13:49:37 -0500
> 
>> 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).

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

I hope we never have a need for anything longer.

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

Re: [PATCH] sparc64: use COMMAND_LINE_SIZE for boot string

[David Miller]

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

> On 10/07/2014 02:03 PM, David Miller wrote:
>> From: Dave Kleikamp <dave.kleikamp@oracle.com>
>> Date: Tue, 07 Oct 2014 13:49:37 -0500
>> 
>>> 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);

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