Re: [Suggestion] PowerPC: kernel: memory access violation whenrtas_data_buf contents are more than 1026

[Benjamin Herrenschmidt]

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 }


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/