Hi Anton,

We also saw a variation of this problem last week and I have an alternative
patch that I'd prefer over this one.  I'll post the patch in a separate
email.

Thanks,

-- 
Wayne Boyer
IBM - Beaverton, Oregon
LTC S/W Development - eServerIO
(503) 578-5236, T/L 775-5236


On 06/02/2010 04:16 AM, Anton Blanchard wrote:
> 
> Victor reported an oops during boot with 2.6.34 on a POWER6 JS22:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=16089
> 
> Checking ipr microcode levels
> Unable to handle kernel paging request for instruction fetch
> Faulting instruction address: 0x322d30312d31302c
> ...
> Oops: Kernel access of bad area, sig: 11 [#1]
> SMP NR_CPUS=128 NUMA pSeries
> last sysfs file:
> /sys/devices/pci0000:00/0000:00:01.0/host0/target0:255:255/0:255:255:255/resource_path
> Modules linked in:
> NIP: 322d30312d31302c LR: 322d30312d31302d CTR: c000000000375bec
> REGS: c0000003d360f8f0 TRAP: 0400   Not tainted  (2.6.34-vjj)
> MSR: 8000000040009432 <EE,ME,IR,DR>  CR: 28002484  XER: 20000020
> TASK = c0000003d587d010[5163] 'iprupdate' THREAD: c0000003d360c000 CPU: 7
> GPR00: 322d30312d31302d c0000003d360fb70 c000000000ad07c0 00000000000185a0 
> GPR04: 0000000000000001 c0000003d360fb10 04000affffffffff c0000000006a6700 
> GPR08: c000000000823383 0000000000000000 0000000000000020 0000000000000000 
> GPR12: 000000000000f032 c00000000f622e00 00000000000000ed 0000000000000000 
> GPR16: 00000000100b8808 0000000010020000 0000000010020000 0000000010010000 
> GPR20: 0000000010010000 0000000000000001 0000000000001000 000000001045eef8 
> GPR24: c0000003d360fdf8 302d31342d31302d 43302d30302d3030 2d30332d44352d34 
> GPR28: 422d33382d30302d 30302d30302d3030 2d30302d30302d30 302d30302d30302d 
> NIP [322d30312d31302c] 0x322d30312d31302c
> LR [322d30312d31302d] 0x322d30312d31302d
> 
> A stack overflow. It turns out ipr_format_resource_path writes to a
> passed in buffer using sprintf which is dangerous and in this case looks
> to be the cause of the overflow.
> 
> The following patch passes in the length of the buffer and uses snprintf,
> which fixes the fail for me. It doesn't fix the other issue, which is
> why the loop in ipr_format_resource_path didn't terminate in the first place.
> That can be fixed in a follow up patch.
> 
> Signed-off-by: Anton Blanchard <an...@samba.org>
> ---
> 
> Index: linux.trees.git/drivers/scsi/ipr.c
> ===================================================================
> --- linux.trees.git.orig/drivers/scsi/ipr.c   2010-05-31 08:51:20.000000000 
> +1000
> +++ linux.trees.git/drivers/scsi/ipr.c        2010-06-02 21:15:41.000000000 
> +1000
> @@ -1132,17 +1132,20 @@ static int ipr_is_same_device(struct ipr
>   * ipr_format_resource_path - Format the resource path for printing.
>   * @res_path:        resource path
>   * @buf:     buffer
> + * @len:     length of the buffer
>   *
>   * Return value:
>   *   pointer to buffer
>   **/
> -static char *ipr_format_resource_path(u8 *res_path, char *buffer)
> +static char *ipr_format_resource_path(u8 *res_path, char *buffer,
> +                                   unsigned int len)
>  {
>       int i;
> +     char *p = buffer;
> 
> -     sprintf(buffer, "%02X", res_path[0]);
> +     p += snprintf(p, buffer + len - p, "%02X", res_path[0]);
>       for (i=1; res_path[i] != 0xff; i++)
> -             sprintf(buffer, "%s-%02X", buffer, res_path[i]);
> +             p += snprintf(p, buffer + len - p, "-%02X", res_path[i]);
> 
>       return buffer;
>  }
> @@ -1187,7 +1190,8 @@ static void ipr_update_res_entry(struct 
> 
>               if (res->sdev && new_path)
>                       sdev_printk(KERN_INFO, res->sdev, "Resource path: %s\n",
> -                                 ipr_format_resource_path(&res->res_path[0], 
> &buffer[0]));
> +                                 ipr_format_resource_path(&res->res_path[0],
> +                                 &buffer[0], sizeof(buffer)));
>       } else {
>               res->flags = cfgtew->u.cfgte->flags;
>               if (res->flags & IPR_IS_IOA_RESOURCE)
> @@ -1573,7 +1577,8 @@ static void ipr_log_sis64_config_error(s
>               ipr_err_separator;
> 
>               ipr_err("Device %d : %s", i + 1,
> -                      ipr_format_resource_path(&dev_entry->res_path[0], 
> &buffer[0]));
> +                      ipr_format_resource_path(&dev_entry->res_path[0],
> +                      &buffer[0], sizeof(buffer)));
>               ipr_log_ext_vpd(&dev_entry->vpd);
> 
>               ipr_err("-----New Device Information-----\n");
> @@ -1919,13 +1924,15 @@ static void ipr_log64_fabric_path(struct
> 
>                       ipr_hcam_err(hostrcb, "%s %s: Resource Path=%s\n",
>                                    path_active_desc[i].desc, 
> path_state_desc[j].desc,
> -                                  
> ipr_format_resource_path(&fabric->res_path[0], &buffer[0]));
> +                                  
> ipr_format_resource_path(&fabric->res_path[0],
> +                                  &buffer[0], sizeof(buffer)));
>                       return;
>               }
>       }
> 
>       ipr_err("Path state=%02X Resource Path=%s\n", path_state,
> -             ipr_format_resource_path(&fabric->res_path[0], &buffer[0]));
> +             ipr_format_resource_path(&fabric->res_path[0], &buffer[0],
> +             sizeof(buffer)));
>  }
> 
>  static const struct {
> @@ -2066,7 +2073,9 @@ static void ipr_log64_path_elem(struct i
> 
>                       ipr_hcam_err(hostrcb, "%s %s: Resource Path=%s, Link 
> rate=%s, WWN=%08X%08X\n",
>                                    path_status_desc[j].desc, 
> path_type_desc[i].desc,
> -                                  
> ipr_format_resource_path(&cfg->res_path[0], &buffer[0]),
> +                                  ipr_format_resource_path(&cfg->res_path[0],
> +                                                           &buffer[0],
> +                                                           sizeof(buffer)),
>                                    link_rate[cfg->link_rate & 
> IPR_PHY_LINK_RATE_MASK],
>                                    be32_to_cpu(cfg->wwid[0]), 
> be32_to_cpu(cfg->wwid[1]));
>                       return;
> @@ -2074,7 +2083,8 @@ static void ipr_log64_path_elem(struct i
>       }
>       ipr_hcam_err(hostrcb, "Path element=%02X: Resource Path=%s, Link 
> rate=%s "
>                    "WWN=%08X%08X\n", cfg->type_status,
> -                  ipr_format_resource_path(&cfg->res_path[0], &buffer[0]),
> +                  ipr_format_resource_path(&cfg->res_path[0], &buffer[0],
> +                  sizeof(buffer)),
>                    link_rate[cfg->link_rate & IPR_PHY_LINK_RATE_MASK],
>                    be32_to_cpu(cfg->wwid[0]), be32_to_cpu(cfg->wwid[1]));
>  }
> @@ -2139,7 +2149,8 @@ static void ipr_log_sis64_array_error(st
> 
>       ipr_err("RAID %s Array Configuration: %s\n",
>               error->protection_level,
> -             ipr_format_resource_path(&error->last_res_path[0], &buffer[0]));
> +             ipr_format_resource_path(&error->last_res_path[0], &buffer[0],
> +                                      sizeof(buffer)));
> 
>       ipr_err_separator;
> 
> @@ -2160,9 +2171,12 @@ static void ipr_log_sis64_array_error(st
>               ipr_err("Array Member %d:\n", i);
>               ipr_log_ext_vpd(&array_entry->vpd);
>               ipr_err("Current Location: %s",
> -                      ipr_format_resource_path(&array_entry->res_path[0], 
> &buffer[0]));
> +                      ipr_format_resource_path(&array_entry->res_path[0],
> +                                               &buffer[0],
> +                                               sizeof(buffer)));
>               ipr_err("Expected Location: %s",
> -                      
> ipr_format_resource_path(&array_entry->expected_res_path[0], &buffer[0]));
> +                      
> ipr_format_resource_path(&array_entry->expected_res_path[0],
> +                                               &buffer[0], sizeof(buffer)));
> 
>               ipr_err_separator;
>       }
> @@ -4099,7 +4113,9 @@ static ssize_t ipr_show_resource_path(st
>       res = (struct ipr_resource_entry *)sdev->hostdata;
>       if (res)
>               len = snprintf(buf, PAGE_SIZE, "%s\n",
> -                            ipr_format_resource_path(&res->res_path[0], 
> &buffer[0]));
> +                            ipr_format_resource_path(&res->res_path[0],
> +                                                     &buffer[0],
> +                                                     sizeof(buffer)));
>       spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags);
>       return len;
>  }
> @@ -4351,7 +4367,9 @@ static int ipr_slave_configure(struct sc
>                       scsi_adjust_queue_depth(sdev, 0, 
> sdev->host->cmd_per_lun);
>               if (ioa_cfg->sis64)
>                       sdev_printk(KERN_INFO, sdev, "Resource path: %s\n",
> -                                 ipr_format_resource_path(&res->res_path[0], 
> &buffer[0]));
> +                                 ipr_format_resource_path(&res->res_path[0],
> +                                                          &buffer[0],
> +                                                          sizeof(buffer)));
>               return 0;
>       }
>       spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags);
> Index: linux.trees.git/drivers/scsi/ipr.h
> ===================================================================
> --- linux.trees.git.orig/drivers/scsi/ipr.h   2010-05-31 08:51:20.000000000 
> +1000
> +++ linux.trees.git/drivers/scsi/ipr.h        2010-06-02 21:15:41.000000000 
> +1000
> @@ -1685,7 +1685,8 @@ struct ipr_ucode_image_header {
>               if ((hostrcb)->ioa_cfg->sis64) {                        \
>                       printk(KERN_ERR IPR_NAME ": %s: " fmt,          \
>                               
> ipr_format_resource_path(&hostrcb->hcam.u.error64.fd_res_path[0], \
> -                                     &hostrcb->rp_buffer[0]),        \
> +                                     &hostrcb->rp_buffer[0],         \
> +                                     sizeof(hostrcb->rp_buffer)),    \
>                               __VA_ARGS__);                           \
>               } else {                                                \
>                       ipr_ra_err((hostrcb)->ioa_cfg,                  \
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Reply via email to