Re: [PATCH 00/10] scsi: avoid linebreaks in syslog output
On 10/15/2012 02:19 AM, Mike Snitzer wrote: On Fri, Oct 12, 2012 at 4:33 AM, Hannes Reinecke wrote: This patchset updates the SCSI midlayer to use dev_printk() instead of the simple printk(). The main objective here is to avoid line-breaks in syslog output; with the current state it's nearly impossible to match the output to the occurring device; under high load even the CDB will be split off into individual bytes, spread randomly across the lines. Which makes debugging via scsi_logging_level _really_ hard. In addition we'll be getting the syslog messages nicely prefixed with the device, which will make userspace logging daemons happy. Before: [ 297.300605] sd 2:0:3:2: Send: [ 297.300607] 0x8802348b0980 [ 297.300610] sd 2:0:3:2: CDB: [ 297.300615] Test Unit Ready: 00 00 00 00 00 00 [ 297.300747] sd 2:0:3:2: Done: [ 297.300750] 0x8802348b0980 SUCCESS [ 297.300753] sd 2:0:3:2: [ 297.300755] Result: hostbyte=DID_OK driverbyte=DRIVER_OK [ 297.300758] sd 2:0:3:2: CDB: [ 297.300764] Test Unit Ready: 00 00 00 00 00 00 [ 297.300766] sd 2:0:3:2: [ 297.300769] Sense Key : Unit Attention [current] [ 297.300771] Info fld=0x0 [ 297.300772] sd 2:0:3:2: [ 297.300776] Add. Sense: Capacity data has changed After: [ 636.683556] sd 2:0:3:2: Send: 0x88043145eec0 [ 636.727856] sd 2:0:3:2: CDB: Test Unit Ready: 00 00 00 00 00 00 [ 636.785330] sd 2:0:3:2: Done: 0x88043145eec0 SUCCESS [ 636.838228] sd 2:0:3:2: Result: hostbyte=DID_OK driverbyte=DRIVER_OK [ 636.899099] sd 2:0:3:2: CDB: Test Unit Ready: 00 00 00 00 00 00 [ 636.955905] sd 2:0:3:2: Sense Key : Unit Attention [current] [ 637.069179] sd 2:0:3:2: Add. Sense: Capacity data has changed I know there are a lot of changes here but shouldn't all this get fixed in stable 3.5+? If you ask me, yes, of course. I can easily post a v2 of the patchset (meanwhile I've got another patch for st :-), Cc'ing stable with it. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- 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
Re: [PATCH 00/10] scsi: avoid linebreaks in syslog output
On Fri, Oct 12, 2012 at 4:33 AM, Hannes Reinecke wrote: > This patchset updates the SCSI midlayer to use dev_printk() instead > of the simple printk(). The main objective here is to avoid line-breaks > in syslog output; with the current state it's nearly impossible to match > the output to the occurring device; under high load even the CDB will > be split off into individual bytes, spread randomly across the lines. > Which makes debugging via scsi_logging_level _really_ hard. > In addition we'll be getting the syslog messages nicely prefixed with > the device, which will make userspace logging daemons happy. > > Before: > [ 297.300605] sd 2:0:3:2: Send: > [ 297.300607] 0x8802348b0980 > [ 297.300610] sd 2:0:3:2: CDB: > [ 297.300615] Test Unit Ready: 00 00 00 00 00 00 > [ 297.300747] sd 2:0:3:2: Done: > [ 297.300750] 0x8802348b0980 SUCCESS > [ 297.300753] sd 2:0:3:2: > [ 297.300755] Result: hostbyte=DID_OK driverbyte=DRIVER_OK > [ 297.300758] sd 2:0:3:2: CDB: > [ 297.300764] Test Unit Ready: 00 00 00 00 00 00 > [ 297.300766] sd 2:0:3:2: > [ 297.300769] Sense Key : Unit Attention [current] > [ 297.300771] Info fld=0x0 > [ 297.300772] sd 2:0:3:2: > [ 297.300776] Add. Sense: Capacity data has changed > > After: > [ 636.683556] sd 2:0:3:2: Send: 0x88043145eec0 > [ 636.727856] sd 2:0:3:2: CDB: Test Unit Ready: 00 00 00 00 00 00 > [ 636.785330] sd 2:0:3:2: Done: 0x88043145eec0 SUCCESS > [ 636.838228] sd 2:0:3:2: Result: hostbyte=DID_OK driverbyte=DRIVER_OK > [ 636.899099] sd 2:0:3:2: CDB: Test Unit Ready: 00 00 00 00 00 00 > [ 636.955905] sd 2:0:3:2: Sense Key : Unit Attention [current] > [ 637.069179] sd 2:0:3:2: Add. Sense: Capacity data has changed I know there are a lot of changes here but shouldn't all this get fixed in stable 3.5+? Thanks for sorting all this out Hannes. -- 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
[Announce] sg3_utils-1.34 available
sg3_utils is a package of command line utilities for sending SCSI and some ATA commands to devices. This package targets the linux kernel (lk) 3, 2.6 and lk 2.4 series. It also has ports to FreeBSD, Tru64, Solaris, and Windows (cygwin and mingw). This version adds sg_xcopy and sg_copy_results (contributed by Hannes Reinecke). This version tracks various changes made by www.t10.org since January 2012. For an overview of sg3_utils and downloads see this page: http://sg.danny.cz/sg/sg3_utils.html The sg_ses utility (for enclosure devices) is discussed at: http://sg.danny.cz/sg/sg_ses.html The SG_IO ioctl is discussed at: http://sg.danny.cz/sg/sg_io.html A full changelog can be found at: http://sg.danny.cz/sg/p/sg3_utils.ChangeLog A release announcement will be sent to freecode.com . Changelog for sg3_utils-1.34 [20121013] [svn: r461] - sg_xcopy: new dd like utility for extended copy command - sg_copy_results: new utility for receive copy results - sg_verify: add 16 byte cdb, bytchk (data-out buffer) and group number support - sync to spc4r36 and sbc3r32 - sg_inq: add --export so sg_inq can replace udev's scsi_id - decode old EMC Symmetrix abuse of VPD page 0x83 - sg_vpd: decode old EMC Symmetrix abuse of VPD page 0x83 - sg_ses: increase max dpage response size to 64 KB - allow ident,locate on enclosure controller - more sanity for additional element status descriptor - sg_sanitize: add --ause, --fail and --test= - sg_luns: add long extended flat space addressing format - sg_logs: add ATA pass-through results lpage (SAT-2) - sg_rtpg: add --extended option - sg_senddiag: list rebuild assist diag page name - sg_pt_linux: expand DID_ (host_byte) codes - cope with a transport error plus sense data - prefer major() over MAJOR() macro - sg_lib: fix sg_get_command_name() service actions - report sdat_ovfl bit (if set) in sense data - decode extended_copy and receive_copy service actions - decode read_buffer and write_buffer modes - decode ATA PT fixed format sense (SAT-2) - sg_cmds_extra: add sg_ll_report_tgt_prt_grp2() - ./configure options: - change --enable-no-linux-bsg to --disable-linuxbsg - add --disable-scsistrings to reduce utility sizes Changelog for sg3_utils-1.33 [20120118] [svn: r435] ... Doug Gilbert -- 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
Re: [SCSI] hpsa: dial down lockup detection during firmware flash
On Wed, 2012-10-10 at 09:18 -0500, scame...@beardog.cce.hp.com wrote: > Yes, I think so. Thanks. I've queued this up for 3.2, thanks. Ben. > -- steve > > On Wed, Oct 10, 2012 at 04:47:38AM +0100, Ben Hutchings wrote: > > Should this fix for hpsa be included in stable updates? It looks like > > it would be needed in 3.2.y and 3.4.y, as lockup detection was > > introduced in 3.2 and the fix went into 3.5. > > > > commit e85c59746957fd6e3595d02cf614370056b5816e > > Author: Stephen M. Cameron > > Date: Tue May 1 11:43:42 2012 -0500 > > > > [SCSI] hpsa: dial down lockup detection during firmware flash > > > > Ben. > > > > -- > > Ben Hutchings > > Who are all these weirdos? - David Bowie, about L-Space IRC channel #afp > > > -- Ben Hutchings Always try to do things in chronological order; it's less confusing that way. signature.asc Description: This is a digitally signed message part
Re: [PATCH] bfa: avoid buffer overrun for 12-byte model name
Jim Meyering wrote:
> Jim Meyering wrote:
>> Krishna Gudipati wrote:
-Original Message-
From: Jim Meyering [mailto:j...@meyering.net]
Sent: Monday, August 20, 2012 9:55 AM
To: linux-ker...@vger.kernel.org
Cc: Jim Meyering; Jing Huang; Krishna Gudipati; James E.J. Bottomley;
linux-
s...@vger.kernel.org
Subject: [PATCH] bfa: avoid buffer overrun for 12-byte model name
From: Jim Meyering
we use strncpy to copy a model name of length up to 15 (16, if you count
the
NUL), into a buffer of size 12 (BFA_FCS_PORT_SYMBNAME_MODEL_SZ).
However, strncpy does not always NUL-terminate, so whenever the original
model string has strlen >= 12, the following strncat reads beyond end of
the -
>sym_name buffer as it attempts to find end of string.
bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric) {
bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);
...
strncpy((char *)&port_cfg->sym_name, model,
BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
strncat((char *)&port_cfg->sym_name,
BFA_FCS_PORT_SYMBNAME_SEPARATOR,
sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
...
bfa_ioc_get_adapter_model(struct bfa_ioc_s *ioc, char *model) {
struct bfi_ioc_attr_s *ioc_attr;
WARN_ON(!model);
memset((void *)model, 0, BFA_ADAPTER_MODEL_NAME_LEN);
BFA_ADAPTER_MODEL_NAME_LEN = 16
Signed-off-by: Jim Meyering
---
drivers/scsi/bfa/bfa_fcs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c index
eaac57e..3329493 100644
--- a/drivers/scsi/bfa/bfa_fcs.c
+++ b/drivers/scsi/bfa/bfa_fcs.c
@@ -713,6 +713,7 @@ bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s
*fabric)
/* Model name/number */
strncpy((char *)&port_cfg->sym_name, model,
BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
+ port_cfg->sym_name[BFA_FCS_PORT_SYMBNAME_MODEL_SZ - 1]
= 0;
strncat((char *)&port_cfg->sym_name,
BFA_FCS_PORT_SYMBNAME_SEPARATOR,
sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
>>>
>>> Nacked-by: Krishna Gudipati
>>>
>>> Hi Jim,
>>>
>>> This model number is of length 12 bytes and the logic added here will
>>> reset the model last byte.
>>> In addition strncat does not need the src to be null terminated, the
>>> change does not compile even.
>>> NACK to this change.
>>
>> Hi Krishna,
>>
>> Thanks for the quick feedback and sorry the patch wasn't quite right.
>> However, the log is accurate: there is at least a theoretical problem
>> when the string in "model" (a buffer of size 16 bytes) has strlen >= 12.
>> While strncat does not require that its second argument be NUL-terminated,
>> the first one (the destination) must be. Otherwise, it has no way to
>> determine the end of the string to which it must append the source bytes.
>
> Ping?
> In case it wasn't clear, there *is* a risk of buffer overflow,
> which happens when strncpy makes it so strncat's destination
> is not NUL terminated.
>
> If you require support for 12-byte model numbers, then
> you'll have to increase the length of that buffer
> (BFA_FCS_PORT_SYMBNAME_MODEL_SZ) to at least 13.
>
> I've just rebased, and thus confirmed that the patches still apply.
>
>> Here is a v2 patch to which I've added the requisite (char*) cast.
>> However, this whole function is rather unreadable due to the
>> repetition (12 times!) of "(char *)&port_cfg->sym_name".
>> In case someone prefers to factor out that repetition,
>> I've appended a larger, v3 patch to do that.
Taking Andi's advice, I've made the offending code use
strlcpy in place of strncpy. More importantly, I've fixed
the same bug also in the following, nearly identical function.
-- >8 --
Two functions have this problem:
bfa_fcs_fabric_psymb_init
bfa_fcs_fabric_nsymb_init
They use strncpy to copy a model name of length up to 15 (16, if you
count the NUL), into a buffer of size 12 (BFA_FCS_PORT_SYMBNAME_MODEL_SZ).
However, strncpy does not always NUL-terminate, so whenever the original
model string has strlen >= 12, the following strncat reads beyond end
of the ->sym_name buffer as it attempts to find end of string.
Instead, use strlcpy, which does guarantee NUL-termination.
bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric)
{
bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);
...
strncpy((char *)&port_cfg->sym_name, model,
BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
strncat((char *)&port_cfg->sym_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
...
bfa_ioc_get_adapter_model(struct bfa_ioc_s *ioc, char *model)
{
struct bfi_ioc_attr_s *ioc_attr;
WARN_ON(!model);
memset((void *)model, 0, BFA_ADAPTER_MODEL_NAME_LEN);
BFA_ADAPTER_MODEL_
Re: [PATCH v3 0/5] Migrate SCSI drivers to use dev_pm_ops
On Friday 12 of October 2012 11:07:18 Alan Stern wrote: > On Fri, 12 Oct 2012, Aaron Lu wrote: > > > v3: > > Only patch 4 is modified: > > Remove the special case for system freeze in scsi_bus_suspend_common > > as pointed out by Alan Stern; > > Updated some comments; > > Removed the use of typedef (*pm_callback_t)(struct device *). > > > > v2: > > Change the runtime suspend behaviour of sd driver by putting the device > > into stopped power state. > > Revert 2 patches which are no longer needed as pointed out by Alan Stern. > > Find out device callbacks in bus callbacks as suggested by Alan Stern. > > > > Due to these changes, patch number grows from 2 -> 5. > > > > v1: > > The 2 patches will migrate SCSI drivers to use the pm callbacks defined > > in dev_pm_ops as pm_message is deprecated and should not be used by driver. > > Bus level callback is changed to use callbacks defined in dev_pm_ops when > > needed and sd's pm callback is updated to use what are defined in > > dev_pm_ops. > > > > Aaron Lu (5): > > sd: put to stopped power state when runtime suspend > > Revert "[SCSI] scsi_pm: set device runtime state before parent > > suspended" > > Revert "[SCSI] runtime resume parent for child's system-resume" > > pm: use callbacks from dev_pm_ops for scsi devices > > sd: update sd to use the new pm callbacks > > > > drivers/scsi/scsi_pm.c | 97 > > +++--- > > drivers/scsi/sd.c | 18 +++--- > > 2 files changed, 66 insertions(+), 49 deletions(-) > > Remember to run all the patches through checkpatch.pl before submitting > them. Aside from that, for the whole series: > > Acked-by: Alan Stern >From me too: Acked-by: Rafael J. Wysocki And please do the checkpatch.pl thing as Alan said. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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
Re: [PATCH v3 4/5] pm: use callbacks from dev_pm_ops for scsi devices
On Friday 12 of October 2012 14:59:37 Aaron Lu wrote:
> Use of pm_message_t is deprecated and device driver is not supposed
> to use that. This patch tries to migrate the SCSI bus level pm callbacks
> to call device's pm callbacks defined in its driver's dev_pm_ops.
Well, as Yoda had said: “Do or do not... there is no try.” ;-)
The patch looks OK, though.
> This is achieved by finding out which device pm callback should be used
> in bus callback function, and then pass that callback function pointer
> as a param to the scsi_bus_{suspend,resume}_common routine, which will
> further pass that callback to scsi_dev_type_{suspend,resume} after
> proper handling.
>
> The special case for freeze in scsi_bus_suspend_common is not necessary
> since there is no high level SCSI driver has implemented freeze, so no
> need to runtime resume the device if it is in runtime suspended state
> for system freeze, just return like the system suspend/hibernate case.
>
> Since only sd has implemented drv->suspend/drv->resume, and I'll update
> sd driver to use the new callbacks in the following patch, there is no
> need to fallback to call drv->suspend/drv->resume if dev_pm_ops is NULL.
>
> Signed-off-by: Aaron Lu
> ---
> drivers/scsi/scsi_pm.c | 85
> ++
> 1 file changed, 52 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index 9923b26..6e70a16 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -16,16 +16,14 @@
>
> #include "scsi_priv.h"
>
> -static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
> +static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device
> *))
> {
> - struct device_driver *drv;
> int err;
>
> err = scsi_device_quiesce(to_scsi_device(dev));
> if (err == 0) {
> - drv = dev->driver;
> - if (drv && drv->suspend) {
> - err = drv->suspend(dev, msg);
> + if (cb) {
> + err = cb(dev);
> if (err)
> scsi_device_resume(to_scsi_device(dev));
> }
> @@ -34,14 +32,12 @@ static int scsi_dev_type_suspend(struct device *dev,
> pm_message_t msg)
> return err;
> }
>
> -static int scsi_dev_type_resume(struct device *dev)
> +static int scsi_dev_type_resume(struct device *dev, int (*cb)(struct device
> *))
> {
> - struct device_driver *drv;
> int err = 0;
>
> - drv = dev->driver;
> - if (drv && drv->resume)
> - err = drv->resume(dev);
> + if (cb)
> + err = cb(dev);
> scsi_device_resume(to_scsi_device(dev));
> dev_dbg(dev, "scsi resume: %d\n", err);
> return err;
> @@ -49,35 +45,33 @@ static int scsi_dev_type_resume(struct device *dev)
>
> #ifdef CONFIG_PM_SLEEP
>
> -static int scsi_bus_suspend_common(struct device *dev, pm_message_t msg)
> +static int
> +scsi_bus_suspend_common(struct device *dev, int (*cb)(struct device *))
> {
> int err = 0;
>
> if (scsi_is_sdev_device(dev)) {
> /*
> - * sd is the only high-level SCSI driver to implement runtime
> - * PM, and sd treats runtime suspend, system suspend, and
> - * system hibernate identically (but not system freeze).
> + * All the high-level SCSI drivers that implement runtime
> + * PM treat runtime suspend, system suspend, and system
> + * hibernate identically.
>*/
> - if (pm_runtime_suspended(dev)) {
> - if (msg.event == PM_EVENT_SUSPEND ||
> - msg.event == PM_EVENT_HIBERNATE)
> - return 0; /* already suspended */
> + if (pm_runtime_suspended(dev))
> + return 0;
>
> - /* wake up device so that FREEZE will succeed */
> - pm_runtime_resume(dev);
> - }
> - err = scsi_dev_type_suspend(dev, msg);
> + err = scsi_dev_type_suspend(dev, cb);
> }
> +
> return err;
> }
>
> -static int scsi_bus_resume_common(struct device *dev)
> +static int
> +scsi_bus_resume_common(struct device *dev, int (*cb)(struct device *))
> {
> int err = 0;
>
> if (scsi_is_sdev_device(dev))
> - err = scsi_dev_type_resume(dev);
> + err = scsi_dev_type_resume(dev, cb);
>
> if (err == 0) {
> pm_runtime_disable(dev);
> @@ -102,26 +96,49 @@ static int scsi_bus_prepare(struct device *dev)
>
> static int scsi_bus_suspend(struct device *dev)
> {
> - return scsi_bus_suspend_common(dev, PMSG_SUSPEND);
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + return scsi_bus_suspend_common(dev, pm ? pm->suspend : NULL);
> +}
> +
> +static int scsi_bus_resume(struct device *dev)
> +{
> +
Re: [PATCH] bfa: avoid buffer overrun for 12-byte model name
Jim Meyering wrote:
> Krishna Gudipati wrote:
>>> -Original Message-
>>> From: Jim Meyering [mailto:j...@meyering.net]
>>> Sent: Monday, August 20, 2012 9:55 AM
>>> To: linux-ker...@vger.kernel.org
>>> Cc: Jim Meyering; Jing Huang; Krishna Gudipati; James E.J. Bottomley; linux-
>>> s...@vger.kernel.org
>>> Subject: [PATCH] bfa: avoid buffer overrun for 12-byte model name
>>>
>>> From: Jim Meyering
>>>
>>> we use strncpy to copy a model name of length up to 15 (16, if you count the
>>> NUL), into a buffer of size 12 (BFA_FCS_PORT_SYMBNAME_MODEL_SZ).
>>> However, strncpy does not always NUL-terminate, so whenever the original
>>> model string has strlen >= 12, the following strncat reads beyond end of
>>> the -
>>> >sym_name buffer as it attempts to find end of string.
>>>
>>> bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric) {
>>> bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);
>>> ...
>>> strncpy((char *)&port_cfg->sym_name, model,
>>> BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
>>> strncat((char *)&port_cfg->sym_name,
>>> BFA_FCS_PORT_SYMBNAME_SEPARATOR,
>>> sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
>>> ...
>>>
>>> bfa_ioc_get_adapter_model(struct bfa_ioc_s *ioc, char *model) {
>>> struct bfi_ioc_attr_s *ioc_attr;
>>>
>>> WARN_ON(!model);
>>> memset((void *)model, 0, BFA_ADAPTER_MODEL_NAME_LEN);
>>>
>>> BFA_ADAPTER_MODEL_NAME_LEN = 16
>>>
>>> Signed-off-by: Jim Meyering
>>> ---
>>> drivers/scsi/bfa/bfa_fcs.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c index
>>> eaac57e..3329493 100644
>>> --- a/drivers/scsi/bfa/bfa_fcs.c
>>> +++ b/drivers/scsi/bfa/bfa_fcs.c
>>> @@ -713,6 +713,7 @@ bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s
>>> *fabric)
>>> /* Model name/number */
>>> strncpy((char *)&port_cfg->sym_name, model,
>>> BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
>>> + port_cfg->sym_name[BFA_FCS_PORT_SYMBNAME_MODEL_SZ - 1]
>>> = 0;
>>> strncat((char *)&port_cfg->sym_name,
>>> BFA_FCS_PORT_SYMBNAME_SEPARATOR,
>>> sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
>>
>> Nacked-by: Krishna Gudipati
>>
>> Hi Jim,
>>
>> This model number is of length 12 bytes and the logic added here will
>> reset the model last byte.
>> In addition strncat does not need the src to be null terminated, the
>> change does not compile even.
>> NACK to this change.
>
> Hi Krishna,
>
> Thanks for the quick feedback and sorry the patch wasn't quite right.
> However, the log is accurate: there is at least a theoretical problem
> when the string in "model" (a buffer of size 16 bytes) has strlen >= 12.
> While strncat does not require that its second argument be NUL-terminated,
> the first one (the destination) must be. Otherwise, it has no way to
> determine the end of the string to which it must append the source bytes.
Ping?
In case it wasn't clear, there *is* a risk of buffer overflow,
which happens when strncpy makes it so strncat's destination
is not NUL terminated.
If you require support for 12-byte model numbers, then
you'll have to increase the length of that buffer
(BFA_FCS_PORT_SYMBNAME_MODEL_SZ) to at least 13.
I've just rebased, and thus confirmed that the patches still apply.
> Here is a v2 patch to which I've added the requisite (char*) cast.
> However, this whole function is rather unreadable due to the
> repetition (12 times!) of "(char *)&port_cfg->sym_name".
> In case someone prefers to factor out that repetition,
> I've appended a larger, v3 patch to do that.
>
> From 4d1ce4e5caf8a5041e5c4f3ae4deddb79c9e247c Mon Sep 17 00:00:00 2001
> From: Jim Meyering
> Date: Sun, 29 Apr 2012 10:41:05 +0200
> Subject: [PATCHv2] bfa: avoid buffer overrun for 12-byte model name
>
> we use strncpy to copy a model name of length up to 15 (16, if you count
> the NUL), into a buffer of size 12 (BFA_FCS_PORT_SYMBNAME_MODEL_SZ).
> However, strncpy does not always NUL-terminate, so whenever the original
> model string has strlen >= 12, the following strncat reads beyond end
> of the ->sym_name buffer as it attempts to find end of string.
>
> bfa_fcs_fabric_psymb_init(struct bfa_fcs_fabric_s *fabric)
> {
> bfa_ioc_get_adapter_model(&fabric->fcs->bfa->ioc, model);
> ...
> strncpy((char *)&port_cfg->sym_name, model,
> BFA_FCS_PORT_SYMBNAME_MODEL_SZ);
> strncat((char *)&port_cfg->sym_name, BFA_FCS_PORT_SYMBNAME_SEPARATOR,
> sizeof(BFA_FCS_PORT_SYMBNAME_SEPARATOR));
> ...
>
> bfa_ioc_get_adapter_model(struct bfa_ioc_s *ioc, char *model)
> {
> struct bfi_ioc_attr_s *ioc_attr;
>
> WARN_ON(!model);
> memset((void *)model, 0, BFA_ADAPTER_MODEL_NAME_LEN);
>
> BFA_ADAPTER_MODEL_NAME_LEN = 16
>
> Signed-off-by: Jim Meyering
> ---
> drivers/scsi/bfa/bfa_fcs.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
> index eaac57e..242c37f 100
