Patchwork [Suggestion] PowerPC: kernel: memory access violation when rtas_data_buf contents are more than 1026

login
register
mail settings
Submitter Chen Gang
Date April 18, 2013, 4:45 a.m.
Message ID <516F7A7D.60206@asianux.com>
Download mbox | patch
Permalink /patch/237429/
State Accepted, archived
Commit 5676005acf26ab7e924a8438ea4746e47d405762
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Chen Gang - April 18, 2013, 4:45 a.m.
Hello Maintainers:


in arch/powerpc/kernel/lparcfg.c, parse_system_parameter_string()

  need set '\0' for 'local_buffer'.

  the reason is:
    SPLPAR_MAXLENGTH is 1026, RTAS_DATA_BUF_SIZE is 4096
    the contents of rtas_data_buf may truncated in memcpy (line 301).

    if contents are truncated.
      the splpar_strlen is more than 1026 (line 321)
      the while loop checking will not find the end of buffer (line 326)
      it will cause memory access violation.


  I find it by reading code, so please help check.

  thanks.

gchen.

-------------------------related fix patch--------------------------------------




-------------------------related source code------------------------------------


283 static void parse_system_parameter_string(struct seq_file *m)
284 {
285         int call_status;
286 
287         unsigned char *local_buffer = kmalloc(SPLPAR_MAXLENGTH, GFP_KERNEL);
288         if (!local_buffer) {
289                 printk(KERN_ERR "%s %s kmalloc failure at line %d\n",
290                        __FILE__, __func__, __LINE__);
291                 return;
292         }
293 
294         spin_lock(&rtas_data_buf_lock);
295         memset(rtas_data_buf, 0, SPLPAR_MAXLENGTH);
296         call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
297                                 NULL,
298                                 SPLPAR_CHARACTERISTICS_TOKEN,
299                                 __pa(rtas_data_buf),
300                                 RTAS_DATA_BUF_SIZE);
301         memcpy(local_buffer, rtas_data_buf, SPLPAR_MAXLENGTH);
302         spin_unlock(&rtas_data_buf_lock);
303         
304         if (call_status != 0) {
305                 printk(KERN_INFO
306                        "%s %s Error calling get-system-parameter (0x%x)\n",
307                        __FILE__, __func__, call_status);
308         } else {       
309                 int splpar_strlen;
310                 int idx, w_idx;
311                 char *workbuffer = kzalloc(SPLPAR_MAXLENGTH, GFP_KERNEL);
312                 if (!workbuffer) { 
313                         printk(KERN_ERR "%s %s kmalloc failure at line %d\n",
314                                __FILE__, __func__, __LINE__);
315                         kfree(local_buffer);
316                         return;
317                 }       
318 #ifdef LPARCFG_DEBUG
319                 printk(KERN_INFO "success calling get-system-parameter\n");
320 #endif
321                 splpar_strlen = local_buffer[0] * 256 + local_buffer[1];
322                 local_buffer += 2;      /* step over strlen value */
323 
324                 w_idx = 0;
325                 idx = 0;
326                 while ((*local_buffer) && (idx < splpar_strlen)) {
327                         workbuffer[w_idx++] = local_buffer[idx++];
328                         if ((local_buffer[idx] == ',')
329                             || (local_buffer[idx] == '\0')) {
330                                 workbuffer[w_idx] = '\0';
331                                 if (w_idx) {
332                                         /* avoid the empty string */
333                                         seq_printf(m, "%s\n", workbuffer);
334                                 }
335                                 memset(workbuffer, 0, SPLPAR_MAXLENGTH);
336                                 idx++;  /* skip the comma */
337                                 w_idx = 0;
338                         } else if (local_buffer[idx] == '=') {
339                                 /* code here to replace workbuffer contents
340                                    with different keyword strings */
341                                 if (0 == strcmp(workbuffer, "MaxEntCap")) {
342                                         strcpy(workbuffer,
343                                                "partition_max_entitled_capacity");
344                                         w_idx = strlen(workbuffer);
345                                 }
346                                 if (0 == strcmp(workbuffer, "MaxPlatProcs")) {
347                                         strcpy(workbuffer,
348                                                "system_potential_processors");
349                                         w_idx = strlen(workbuffer);
350                                 }
351                         }
352                 }
353                 kfree(workbuffer);
354                 local_buffer -= 2;      /* back up over strlen value */
355         }
356         kfree(local_buffer);
357 }
Benjamin Herrenschmidt - April 23, 2013, 12:31 a.m.
On Thu, 2013-04-18 at 12:45 +0800, Chen Gang wrote:
> Hello Maintainers:
> 
> 
> in arch/powerpc/kernel/lparcfg.c, parse_system_parameter_string()
> 
>   need set '\0' for 'local_buffer'.
> 
>   the reason is:
>     SPLPAR_MAXLENGTH is 1026, RTAS_DATA_BUF_SIZE is 4096
>     the contents of rtas_data_buf may truncated in memcpy (line 301).
> 
>     if contents are truncated.
>       the splpar_strlen is more than 1026 (line 321)
>       the while loop checking will not find the end of buffer (line 326)
>       it will cause memory access violation.
> 
> 
>   I find it by reading code, so please help check.

And a signed-off-by please ?

Cheers,
Ben.

>   thanks.
> 
> gchen.
> 
> -------------------------related fix patch--------------------------------------
> 
> diff --git a/arch/powerpc/kernel/lparcfg.c b/arch/powerpc/kernel/lparcfg.c
> index 801a757..d92f387 100644
> --- a/arch/powerpc/kernel/lparcfg.c
> +++ b/arch/powerpc/kernel/lparcfg.c
> @@ -299,6 +299,7 @@ static void parse_system_parameter_string(struct seq_file *m)
>  				__pa(rtas_data_buf),
>  				RTAS_DATA_BUF_SIZE);
>  	memcpy(local_buffer, rtas_data_buf, SPLPAR_MAXLENGTH);
> +	local_buffer[SPLPAR_MAXLENGTH - 1] = '\0';
>  	spin_unlock(&rtas_data_buf_lock);
>  
>  	if (call_status != 0) {
> 
> 
> 
> -------------------------related source code------------------------------------
> 
> 
> 283 static void parse_system_parameter_string(struct seq_file *m)
> 284 {
> 285         int call_status;
> 286 
> 287         unsigned char *local_buffer = kmalloc(SPLPAR_MAXLENGTH, GFP_KERNEL);
> 288         if (!local_buffer) {
> 289                 printk(KERN_ERR "%s %s kmalloc failure at line %d\n",
> 290                        __FILE__, __func__, __LINE__);
> 291                 return;
> 292         }
> 293 
> 294         spin_lock(&rtas_data_buf_lock);
> 295         memset(rtas_data_buf, 0, SPLPAR_MAXLENGTH);
> 296         call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
> 297                                 NULL,
> 298                                 SPLPAR_CHARACTERISTICS_TOKEN,
> 299                                 __pa(rtas_data_buf),
> 300                                 RTAS_DATA_BUF_SIZE);
> 301         memcpy(local_buffer, rtas_data_buf, SPLPAR_MAXLENGTH);
> 302         spin_unlock(&rtas_data_buf_lock);
> 303         
> 304         if (call_status != 0) {
> 305                 printk(KERN_INFO
> 306                        "%s %s Error calling get-system-parameter (0x%x)\n",
> 307                        __FILE__, __func__, call_status);
> 308         } else {       
> 309                 int splpar_strlen;
> 310                 int idx, w_idx;
> 311                 char *workbuffer = kzalloc(SPLPAR_MAXLENGTH, GFP_KERNEL);
> 312                 if (!workbuffer) { 
> 313                         printk(KERN_ERR "%s %s kmalloc failure at line %d\n",
> 314                                __FILE__, __func__, __LINE__);
> 315                         kfree(local_buffer);
> 316                         return;
> 317                 }       
> 318 #ifdef LPARCFG_DEBUG
> 319                 printk(KERN_INFO "success calling get-system-parameter\n");
> 320 #endif
> 321                 splpar_strlen = local_buffer[0] * 256 + local_buffer[1];
> 322                 local_buffer += 2;      /* step over strlen value */
> 323 
> 324                 w_idx = 0;
> 325                 idx = 0;
> 326                 while ((*local_buffer) && (idx < splpar_strlen)) {
> 327                         workbuffer[w_idx++] = local_buffer[idx++];
> 328                         if ((local_buffer[idx] == ',')
> 329                             || (local_buffer[idx] == '\0')) {
> 330                                 workbuffer[w_idx] = '\0';
> 331                                 if (w_idx) {
> 332                                         /* avoid the empty string */
> 333                                         seq_printf(m, "%s\n", workbuffer);
> 334                                 }
> 335                                 memset(workbuffer, 0, SPLPAR_MAXLENGTH);
> 336                                 idx++;  /* skip the comma */
> 337                                 w_idx = 0;
> 338                         } else if (local_buffer[idx] == '=') {
> 339                                 /* code here to replace workbuffer contents
> 340                                    with different keyword strings */
> 341                                 if (0 == strcmp(workbuffer, "MaxEntCap")) {
> 342                                         strcpy(workbuffer,
> 343                                                "partition_max_entitled_capacity");
> 344                                         w_idx = strlen(workbuffer);
> 345                                 }
> 346                                 if (0 == strcmp(workbuffer, "MaxPlatProcs")) {
> 347                                         strcpy(workbuffer,
> 348                                                "system_potential_processors");
> 349                                         w_idx = strlen(workbuffer);
> 350                                 }
> 351                         }
> 352                 }
> 353                 kfree(workbuffer);
> 354                 local_buffer -= 2;      /* back up over strlen value */
> 355         }
> 356         kfree(local_buffer);
> 357 }
Chen Gang - April 23, 2013, 1:48 a.m.
On 2013年04月23日 08:31, Benjamin Herrenschmidt wrote:
> On Thu, 2013-04-18 at 12:45 +0800, Chen Gang wrote:
>> Hello Maintainers:
>>
>>
>> in arch/powerpc/kernel/lparcfg.c, parse_system_parameter_string()
>>
>>   need set '\0' for 'local_buffer'.
>>
>>   the reason is:
>>     SPLPAR_MAXLENGTH is 1026, RTAS_DATA_BUF_SIZE is 4096
>>     the contents of rtas_data_buf may truncated in memcpy (line 301).
>>
>>     if contents are truncated.
>>       the splpar_strlen is more than 1026 (line 321)
>>       the while loop checking will not find the end of buffer (line 326)
>>       it will cause memory access violation.
>>
>>
>>   I find it by reading code, so please help check.
> 
> And a signed-off-by please ?
> 

  ok, thanks, I should send the related patch.

Patch

diff --git a/arch/powerpc/kernel/lparcfg.c b/arch/powerpc/kernel/lparcfg.c
index 801a757..d92f387 100644
--- a/arch/powerpc/kernel/lparcfg.c
+++ b/arch/powerpc/kernel/lparcfg.c
@@ -299,6 +299,7 @@  static void parse_system_parameter_string(struct seq_file *m)
 				__pa(rtas_data_buf),
 				RTAS_DATA_BUF_SIZE);
 	memcpy(local_buffer, rtas_data_buf, SPLPAR_MAXLENGTH);
+	local_buffer[SPLPAR_MAXLENGTH - 1] = '\0';
 	spin_unlock(&rtas_data_buf_lock);
 
 	if (call_status != 0) {