Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places
On 09/05/2019 16:42, James Bottomley wrote: > On Thu, 2019-05-09 at 17:30 +0200, Hannes Reinecke wrote: >> On 5/8/19 4:24 PM, James Bottomley wrote: >>> On Wed, 2019-05-08 at 14:07 +0100, Colin Ian King wrote: >>>> On 05/05/2019 04:34, James Bottomley wrote: >>>>> On Sat, 2019-05-04 at 17:40 +0100, Colin King wrote: >>>>>> From: Colin Ian King >>>>>> >>>>>> Currently the shift of int value 1 by more than 31 places can >>>>>> result in undefined behaviour. Fix this by making the 1 a ULL >>>>>> value before the shift operation. >>>>> >>>>> Fusion SAS is pretty ancient. I thought the largest one ever >>>>> produced had four phys, so how did you produce the overflow? >>>> >>>> This was an issue found by static analysis with Coverity; so I >>>> guess won't happen in the wild, in which case the patch could be >>>> ignored. >>> >>> The point I was more making is that if we thought this could ever >>> happen in practice, we'd need more error handling than simply this: >>> we'd be setting the phy_bitmap to zero which would be every bit as >>> bad as some random illegal value. >>> >> >> Thing is, mptsas is used as the default emulation in VMWare, and >> that does allow you to do some pretty weird configurations (I've >> found myself fixing a bug with SATA hotplug on mptsas once ...). >> So I wouldn't discard this issue out of hand. > > I'm not, I'm just saying the proposed fix is no fix at all since it > would just produce undefined behaviour in the driver. I thought the > issue might have been coming from VMWare, which is why I asked how the > bug was seen. The proper fix is probably to fail attachment if the phy > number goes over a fixed value (16 sounds reasonable) but if it's never > a problem in the field, I'm happy doing nothing because we have no real > idea what the reasonable value is. I'm happy with my proposed fix being ignored and (if required) a more appropriate fix developed by somebody who is more familiar with this code than me. Sometimes it is hard to determine if these static analysis "issues" are fix worthy or not. Colin > > James >
Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places
On 05/05/2019 04:34, James Bottomley wrote: > On Sat, 2019-05-04 at 17:40 +0100, Colin King wrote: >> From: Colin Ian King >> >> Currently the shift of int value 1 by more than 31 places can result >> in undefined behaviour. Fix this by making the 1 a ULL value before >> the shift operation. > > Fusion SAS is pretty ancient. I thought the largest one ever produced > had four phys, so how did you produce the overflow? This was an issue found by static analysis with Coverity; so I guess won't happen in the wild, in which case the patch could be ignored. Colin > > James >
Re: [PATCH] scsi: qedf: replace memset/memcpy with safer strscpy
On 12/04/2019 10:20, YueHaibing wrote:
>
> I send a similar fix earlier, but it seems not be picked.
It would be useful to get a fix for this applied as I'm seeing many
multiple of reports of this issue with Coverity and it is a relatively
simple fix.
Colin
>
> https://patchwork.kernel.org/patch/10874565/
>
> On 2019/4/12 17:05, Colin King wrote:
>> From: Colin Ian King
>>
>> Currently the qedf_dbg_* family of functions can overrun the end
>> of the source string if it is less than the destination buffer
>> length because of the use of a fixed sized memcpy. Replace the
>> memset/memcpy calls with the safer strscpy as this won't overrun
>> the end of the source string and it ensures the destination
>> string is always terminated with NUL.
>>
>> Addresses-Coverity: ("Out-of-bounds access")
>> Fixes: 61d8658b4a43 ("scsi: qedf: Add QLogic FastLinQ offload FCoE driver
>> framework.")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/scsi/qedf/qedf_dbg.c | 12
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/qedf/qedf_dbg.c b/drivers/scsi/qedf/qedf_dbg.c
>> index f2397ee9ba69..b491bebeaca9 100644
>> --- a/drivers/scsi/qedf/qedf_dbg.c
>> +++ b/drivers/scsi/qedf/qedf_dbg.c
>> @@ -17,8 +17,7 @@ qedf_dbg_err(struct qedf_dbg_ctx *qedf, const char *func,
>> u32 line,
>> struct va_format vaf;
>> char nfunc[32];
>>
>> -memset(nfunc, 0, sizeof(nfunc));
>> -memcpy(nfunc, func, sizeof(nfunc) - 1);
>> +strscpy(nfunc, func, sizeof(nfunc));
>>
>> va_start(va, fmt);
>>
>> @@ -42,8 +41,7 @@ qedf_dbg_warn(struct qedf_dbg_ctx *qedf, const char *func,
>> u32 line,
>> struct va_format vaf;
>> char nfunc[32];
>>
>> -memset(nfunc, 0, sizeof(nfunc));
>> -memcpy(nfunc, func, sizeof(nfunc) - 1);
>> +strscpy(nfunc, func, sizeof(nfunc));
>>
>> va_start(va, fmt);
>>
>> @@ -71,8 +69,7 @@ qedf_dbg_notice(struct qedf_dbg_ctx *qedf, const char
>> *func, u32 line,
>> struct va_format vaf;
>> char nfunc[32];
>>
>> -memset(nfunc, 0, sizeof(nfunc));
>> -memcpy(nfunc, func, sizeof(nfunc) - 1);
>> +strscpy(nfunc, func, sizeof(nfunc));
>>
>> va_start(va, fmt);
>>
>> @@ -101,8 +98,7 @@ qedf_dbg_info(struct qedf_dbg_ctx *qedf, const char
>> *func, u32 line,
>> struct va_format vaf;
>> char nfunc[32];
>>
>> -memset(nfunc, 0, sizeof(nfunc));
>> -memcpy(nfunc, func, sizeof(nfunc) - 1);
>> +strscpy(nfunc, func, sizeof(nfunc));
>>
>> va_start(va, fmt);
>>
>>
>
Re: [SCSI] pmcraid: PMC-Sierra MaxRAID driver to support 6Gb/s SAS RAID controller
Hi,
Static analysis from CoverityScan (CID#114178 "Operands don't affect
result") detected an issue in drivers/scsi/pmcraid.c, function
pmcraid_init_res_table with the following check:
if (pinstance->cfg_table->flags & MICROCODE_UPDATE_REQUIRED)
pmcraid_err("IOA requires microcode download\n");
pinstance->cfg_table->flags is a u8, MICROCODE_UPDATE_REQUIRED is 1 <<
31, so the & operation always results in false and the error message is
never displayed. From my understanding, flags should be a u8, so there
is something wrong here with the check. Any ideas?
Colin
NAK: [PATCH] scsi: lpfc: fix spelling mistake: "mabilbox" -> "mailbox"
On 03/05/18 10:19, Colin King wrote:
> From: Colin Ian King
>
> Trivial fix to spelling mistake in lpfc_printf_log log message
>
> Signed-off-by: Colin Ian King
Ignore this, I've found more issues, sending a V2.
> ---
> drivers/scsi/lpfc/lpfc_init.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index 060f0e2f6ff5..dc4334fa41bb 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -5117,7 +5117,7 @@ lpfc_sli4_async_fip_evt(struct lpfc_hba *phba,
> if (rc) {
> lpfc_printf_log(phba, KERN_ERR, LOG_FIP |
> LOG_DISCOVERY,
> - "2772 Issue FCF rediscover mabilbox "
> + "2772 Issue FCF rediscover mailbox "
> "command failed, fail through to FCF "
> "dead event\n");
> spin_lock_irq(&phba->hbalock);
> @@ -5209,7 +5209,7 @@ lpfc_sli4_async_fip_evt(struct lpfc_hba *phba,
> lpfc_printf_log(phba, KERN_ERR, LOG_FIP |
> LOG_DISCOVERY,
> "2774 Issue FCF rediscover "
> - "mabilbox command failed, "
> + "mailbox command failed, "
> "through to CVL event\n");
> spin_lock_irq(&phba->hbalock);
> phba->fcf.fcf_flag &= ~FCF_ACVL_DISC;
>
Re: [PATCH] isci: Fix infinite loop in while loop
On 20/04/18 10:45, James Bottomley wrote: > On Fri, 2018-04-20 at 10:03 +0100, Colin King wrote: >> From: Colin Ian King >> >> In the case when the phy_mask is bitwise anded with the >> phy_index bit is zero the continue statement currently jumps >> to the next iteration of the while loop and phy_index is >> never actually incremented, potentially causing an infinite >> loop if phy_index is less than SCI_MAX_PHS. Fix this by >> jumping to the increment of phy_index. >> >> [ The goto is used to save one more level of nesting that >> makes the code far wider than 80 columns. ] > > what's wrong with replacing the while() with a for() that just works > (removing the increment at the end). This is effectively open coding a > for loop anyway, which is a pattern we wouldn't want replicated. > > James > Good point, V2 en-route.
re: scsi: sg: fix SG_DXFER_FROM_DEV transfers
Hi,
minor nit-pick on the following commit:
scsi: sg: fix SG_DXFER_FROM_DEV transfers
SG_DXFER_FROM_DEV transfers do not necessarily have a dxferp as we set
it to NULL for the old sg_io read/write interface, but must have a
length bigger than 0. This fixes a regression introduced by commit
28676d869bbb ("scsi: sg: check for valid direction before starting the
request")
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 21225d62b0c1..1e82d4128a84 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -758,8 +758,11 @@ static bool sg_is_valid_dxfer(sg_io_hdr_t *hp)
if (hp->dxferp || hp->dxfer_len > 0)
return false;
return true;
- case SG_DXFER_TO_DEV:
case SG_DXFER_FROM_DEV:
+ if (hp->dxfer_len < 0)
hp->dxfer_len is an unsigned int so this condition is redundant and
false is never returned. Is that intentional?
+ return false;
+ return true;
+ case SG_DXFER_TO_DEV:
Colin
NAK: [PATCH] scsi: snic: fix a couple of spelling mistakes/typos
Incorrect commit message, I'll resend.
On 30/06/17 14:54, Colin King wrote:
> From: Colin Ian King
>
> Trivial fix to spelling mistakes/typos:
>
> "Allodating" -> "Allocating"
> "incative" -> "inactive"
>
> Signed-off-by: Colin Ian King
> ---
> drivers/scsi/snic/snic_isr.c | 4 ++--
> drivers/scsi/snic/snic_scsi.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/snic/snic_isr.c b/drivers/scsi/snic/snic_isr.c
> index d859501e4ccd..c4da3673f2ae 100644
> --- a/drivers/scsi/snic/snic_isr.c
> +++ b/drivers/scsi/snic/snic_isr.c
> @@ -141,7 +141,7 @@ snic_request_intr(struct snic *snic)
> snic->msix[i].devid);
> if (ret) {
> SNIC_HOST_ERR(snic->shost,
> - "MSI-X: requrest_irq(%d) failed %d\n",
> + "MSI-X: request_irq(%d) failed %d\n",
> i,
> ret);
> snic_free_intr(snic);
> @@ -151,7 +151,7 @@ snic_request_intr(struct snic *snic)
> }
>
> return ret;
> -} /* end of snic_requrest_intr */
> +} /* end of snic_request_intr */
>
> int
> snic_set_intr_mode(struct snic *snic)
> diff --git a/drivers/scsi/snic/snic_scsi.c b/drivers/scsi/snic/snic_scsi.c
> index 05c3a7282d4a..d8a376b7882d 100644
> --- a/drivers/scsi/snic/snic_scsi.c
> +++ b/drivers/scsi/snic/snic_scsi.c
> @@ -1260,7 +1260,7 @@ snic_io_cmpl_handler(struct vnic_dev *vdev,
> default:
> SNIC_BUG_ON(1);
> SNIC_SCSI_DBG(snic->shost,
> - "Unknown Firmwqre completion request type %d\n",
> + "Unknown Firmware completion request type %d\n",
> fwreq->hdr.type);
> break;
> }
>
Re: [PATCH] scsi: lpfc: fix spelling mistake "entrys" -> "entries"
On 01/06/17 15:35, Dan Carpenter wrote: > On Fri, May 26, 2017 at 11:11:37AM +0100, Colin King wrote: >> From: Colin Ian King >> >> Trivial fix to spelling mistake in debugfs message >> > > Are you using a tool to find all these spelling mistakes? Yep, I'm using kernelscan [1] kernelscan -c path-to-kernel-source I run this daily and diff the latest with the previous results. It uses the dictionary from "spell". It's been hand optimized to be rather fast. [1] https://github.com/ColinIanKing/kernelscan > > regards, > dan carpenter >
Re: [PATCH] target/iscsi: make function target_parse_xcopy_cmd static
On 11/05/17 14:55, Bart Van Assche wrote:
> On Thu, 2017-05-11 at 11:16 +0100, Colin King wrote:
>> From: Colin Ian King
>>
>> Making target_parse_xcopy_cmd static fixes sparse warning:
>>
>> "warning: symbol 'target_parse_xcopy_cmd' was not declared. Should
>> it be static?"
>>
>> Fixes: 1bd05294519f76 ("target/iscsi: Fix a deadlock between the XCOPY code
>> and session shutdown")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/target/target_core_xcopy.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/target/target_core_xcopy.c
>> b/drivers/target/target_core_xcopy.c
>> index aecb36d955f0..00e0269e6be4 100644
>> --- a/drivers/target/target_core_xcopy.c
>> +++ b/drivers/target/target_core_xcopy.c
>> @@ -886,7 +886,7 @@ static void target_xcopy_do_work(struct work_struct
>> *work)
>> * Returns TCM_NO_SENSE upon success or a sense code != TCM_NO_SENSE if
>> parsing
>> * fails.
>> */
>> -sense_reason_t target_parse_xcopy_cmd(struct xcopy_op *xop)
>> +static sense_reason_t target_parse_xcopy_cmd(struct xcopy_op *xop)
>> {
>> struct se_cmd *se_cmd = xop->xop_se_cmd;
>> unsigned char *p = NULL, *seg_desc;
>
> Hello Colin,
>
> Thanks for the patch. I will merge it in patch "target/iscsi: Fix a deadlock
> between the
> XCOPY code and session shutdown".
>
> Bart.
>
Thanks, sounds good to me.
Colin
Re: [PATCH] scsi: fcoe: sanity check string size for store_ctrl_mode option
On 22/03/17 19:39, Dan Carpenter wrote:
> On Wed, Mar 22, 2017 at 02:01:37PM +, Colin King wrote:
>> From: Colin Ian King
>>
>> Reading and writing to mode[count - 1] implies the count should not
>> be less than 1 so add a sanity check for this.
>>
>> Detected with CoverityScan, CID#1357345 ("Overflowed array index write")
>>
>> Signed-off-by: Colin Ian King
>
> This is harmless, of course, but count can't be zero. This is a sysfs
> file so we test for zero size writes in sysfs_kf_write() and return
> early.
Ah, thanks for pointing out that. I overlooked that detail.
Colin
>
> regards,
> dan carpenter
>
Re: [PATCH] scsi: aacraid: rcode is unsigned, so can never be less than zero
On 07/02/17 11:37, Dan Carpenter wrote:
> On Tue, Feb 07, 2017 at 11:27:38AM +, Colin King wrote:
>> From: Colin Ian King
>>
>> The check on rcode >= 0 is always true because rcode is unsigned
>> and can never be less than zero. Remove the redundant check.
>>
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/scsi/aacraid/aachba.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
>> index 3b5ddf4..ddfd726 100644
>> --- a/drivers/scsi/aacraid/aachba.c
>> +++ b/drivers/scsi/aacraid/aachba.c
>> @@ -1848,7 +1848,7 @@ int aac_report_phys_luns(struct aac_dev *dev, struct
>> fib *fibptr, int rescan)
>> FsaNormal, 1, 1, NULL, NULL);
>>
>> /* analyse data */
>> -if (rcode >= 0 && phys_luns->resp_flag == 2) {
>
> The original code is buggy. rcode should be an int.
Thanks for spotting that. Oops.
>
> regards,
> dan carpenter
>
Re: [PATCH v1] scsi: ufs: fix arguments order some trace calls
On 11/01/17 00:48, Subhash Jadavani wrote:
> Colin Ian King reported that with
> commit 7ff5ab473633 ("scsi: ufs: add tracing support") static analysis
> is reporting that we may have swapped arguments on calls to:
> trace_ufshcd_runtime_resume,
> trace_ufshcd_runtime_suspend,
> trace_ufshcd_system_suspend,
> trace_ufshcd_system_resume,
> and trace_ufshcd_init
>
> Where:
> hba->uic_link_state is passed to dev_state
> hba->curr_dev_pwr_mode is passed to link_state
>
> This wasn't intentional so it's a bug. This change fixed this bug.
>
> Reported-by: Colin Ian King
> Signed-off-by: Subhash Jadavani
> ---
> drivers/scsi/ufs/ufshcd.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index be6322e..6b56eb0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5805,7 +5805,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>
> trace_ufshcd_init(dev_name(hba->dev), ret,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> - hba->uic_link_state, hba->curr_dev_pwr_mode);
> + hba->curr_dev_pwr_mode, hba->uic_link_state);
> return ret;
> }
>
> @@ -6819,7 +6819,7 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
> out:
> trace_ufshcd_system_suspend(dev_name(hba->dev), ret,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> - hba->uic_link_state, hba->curr_dev_pwr_mode);
> + hba->curr_dev_pwr_mode, hba->uic_link_state);
> if (!ret)
> hba->is_sys_suspended = true;
> return ret;
> @@ -6852,7 +6852,7 @@ int ufshcd_system_resume(struct ufs_hba *hba)
> out:
> trace_ufshcd_system_resume(dev_name(hba->dev), ret,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> - hba->uic_link_state, hba->curr_dev_pwr_mode);
> + hba->curr_dev_pwr_mode, hba->uic_link_state);
> return ret;
> }
> EXPORT_SYMBOL(ufshcd_system_resume);
> @@ -6880,7 +6880,7 @@ int ufshcd_runtime_suspend(struct ufs_hba *hba)
> out:
> trace_ufshcd_runtime_suspend(dev_name(hba->dev), ret,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> - hba->uic_link_state, hba->curr_dev_pwr_mode);
> + hba->curr_dev_pwr_mode, hba->uic_link_state);
> return ret;
> }
> EXPORT_SYMBOL(ufshcd_runtime_suspend);
> @@ -6921,7 +6921,7 @@ int ufshcd_runtime_resume(struct ufs_hba *hba)
> out:
> trace_ufshcd_runtime_resume(dev_name(hba->dev), ret,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> - hba->uic_link_state, hba->curr_dev_pwr_mode);
> + hba->curr_dev_pwr_mode, hba->uic_link_state);
> return ret;
> }
> EXPORT_SYMBOL(ufshcd_runtime_resume);
>
Thanks, looks good to me.
Acked-by: Colin Ian King
--
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
