[PATCH] scsi_devinfo: fixup string compare

[PATCH] scsi_devinfo: fixup string compare

[Hannes Reinecke]

When checking the model and vendor string we need to use the
minimum value of either string, otherwise we'll miss out on
wildcard matches.
Without this patch certain Hitachi arrays will not be presenting
VPD pages correctly.

Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_devinfo.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 28fea83..f8a302f 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -456,11 +456,13 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 			/*
 			 * Behave like the older version of get_device_flags.
 			 */
-			if (memcmp(devinfo->vendor, vskip, vmax) ||
+			if (memcmp(devinfo->vendor, vskip,
+				   min(vmax, strlen(devinfo->vendor))) ||
 					(vmax < sizeof(devinfo->vendor) &&
 						devinfo->vendor[vmax]))
 				continue;
-			if (memcmp(devinfo->model, mskip, mmax) ||
+			if (memcmp(devinfo->model, mskip,
+				   min(mmax, strlen(devinfo->model))) ||
 					(mmax < sizeof(devinfo->model) &&
 						devinfo->model[mmax]))
 				continue;
-- 
1.8.5.6

Re: [PATCH] scsi_devinfo: fixup string compare

[Bart Van Assche]

On Fri, 2017-08-04 at 11:40 +0200, Hannes Reinecke wrote:
> When checking the model and vendor string we need to use the
> minimum value of either string, otherwise we'll miss out on
> wildcard matches.
> Without this patch certain Hitachi arrays will not be presenting
> VPD pages correctly.
> 
> Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi_devinfo.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index 28fea83..f8a302f 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -456,11 +456,13 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
>  			/*
>  			 * Behave like the older version of get_device_flags.
>  			 */
> -			if (memcmp(devinfo->vendor, vskip, vmax) ||
> +			if (memcmp(devinfo->vendor, vskip,
> +				   min(vmax, strlen(devinfo->vendor))) ||
>  					(vmax < sizeof(devinfo->vendor) &&
>  						devinfo->vendor[vmax]))
>  				continue;
> -			if (memcmp(devinfo->model, mskip, mmax) ||
> +			if (memcmp(devinfo->model, mskip,
> +				   min(mmax, strlen(devinfo->model))) ||
>  					(mmax < sizeof(devinfo->model) &&
>  						devinfo->model[mmax]))
>  				continue;

Hello Hannes,

Will this reintroduce the bug mentioned in commit b704f70ce200? The description
of that commit is as follows:

=========================================================================

    SCSI: fix bug in scsi_dev_info_list matching
    
    The "compatible" matching algorithm used for looking up old-style
    blacklist entries in a scsi_dev_info_list is buggy.  The core of the
    algorithm looks like this:
    
                    if (memcmp(devinfo->vendor, vendor,
                                min(max, strlen(devinfo->vendor))))
                            /* not a match */
    
    where max is the length of the device's vendor string after leading
    spaces have been removed but trailing spaces have not.  Because of the
    min() computation, either entry could be a proper substring of the
    other and the code would still think that they match.
    
    In the case originally reported, the device's vendor and product
    strings were "Inateck " and "                ".  These matched against
    the following entry in the global device list:
    
            {"", "Scanner", "1.80", BLIST_NOLUN}
    
    because "" is a substring of "Inateck " and "" (the result of removing
    leading spaces from the device's product string) is a substring of
    "Scanner".  The mistaken match prevented the system from scanning and
    finding the device's second Logical Unit.
    
    This patch fixes the problem by making two changes.  First, the code
    for leading-space removal is hoisted out of the loop.  (This means it
    will sometimes run unnecessarily, but since a large percentage of all
    lookups involve the "compatible" entries in global device list, this
    should be an overall improvement.)  Second and more importantly, the
    patch removes trailing spaces and adds a check to verify that the two
    resulting strings are exactly the same length.  This prevents matches
    where one entry is a proper substring of the other.

=========================================================================

Thanks,

Bart.

Re: [PATCH] scsi_devinfo: fixup string compare

[Hannes Reinecke]

On 08/04/2017 05:32 PM, Bart Van Assche wrote:
> On Fri, 2017-08-04 at 11:40 +0200, Hannes Reinecke wrote:
>> When checking the model and vendor string we need to use the
>> minimum value of either string, otherwise we'll miss out on
>> wildcard matches.
>> Without this patch certain Hitachi arrays will not be presenting
>> VPD pages correctly.
>>
>> Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/scsi_devinfo.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
>> index 28fea83..f8a302f 100644
>> --- a/drivers/scsi/scsi_devinfo.c
>> +++ b/drivers/scsi/scsi_devinfo.c
>> @@ -456,11 +456,13 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
>>  			/*
>>  			 * Behave like the older version of get_device_flags.
>>  			 */
>> -			if (memcmp(devinfo->vendor, vskip, vmax) ||
>> +			if (memcmp(devinfo->vendor, vskip,
>> +				   min(vmax, strlen(devinfo->vendor))) ||
>>  					(vmax < sizeof(devinfo->vendor) &&
>>  						devinfo->vendor[vmax]))
>>  				continue;
>> -			if (memcmp(devinfo->model, mskip, mmax) ||
>> +			if (memcmp(devinfo->model, mskip,
>> +				   min(mmax, strlen(devinfo->model))) ||
>>  					(mmax < sizeof(devinfo->model) &&
>>  						devinfo->model[mmax]))
>>  				continue;
> 
> Hello Hannes,
> 
> Will this reintroduce the bug mentioned in commit b704f70ce200? The description
> of that commit is as follows:
> 
> =========================================================================
> 
>     SCSI: fix bug in scsi_dev_info_list matching
>     
>     The "compatible" matching algorithm used for looking up old-style
>     blacklist entries in a scsi_dev_info_list is buggy.  The core of the
>     algorithm looks like this:
>     
>                     if (memcmp(devinfo->vendor, vendor,
>                                 min(max, strlen(devinfo->vendor))))
>                             /* not a match */
>     
>     where max is the length of the device's vendor string after leading
>     spaces have been removed but trailing spaces have not.  Because of the
>     min() computation, either entry could be a proper substring of the
>     other and the code would still think that they match.
>     
>     In the case originally reported, the device's vendor and product
>     strings were "Inateck " and "                ".  These matched against
>     the following entry in the global device list:
>     
>             {"", "Scanner", "1.80", BLIST_NOLUN}
>     
>     because "" is a substring of "Inateck " and "" (the result of removing
>     leading spaces from the device's product string) is a substring of
>     "Scanner".  The mistaken match prevented the system from scanning and
>     finding the device's second Logical Unit.
>     
>     This patch fixes the problem by making two changes.  First, the code
>     for leading-space removal is hoisted out of the loop.  (This means it
>     will sometimes run unnecessarily, but since a large percentage of all
>     lookups involve the "compatible" entries in global device list, this
>     should be an overall improvement.)  Second and more importantly, the
>     patch removes trailing spaces and adds a check to verify that the two
>     resulting strings are exactly the same length.  This prevents matches
>     where one entry is a proper substring of the other.
> 
Well, maybe; however, the current logic fails to match the entry

	{"HITACHI", "OPEN-", "*", BLIST_REPORTLUN2},

against the 'real' name, which is "HITACHI" "OPEN-V".
And for some reason we have far more customer using Hitachi arrays than
using scanner of dubious provenance with no Vendor (which is the real
bug if you ask me...)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] scsi_devinfo: fixup string compare

[Bart Van Assche]

On Fri, 2017-08-04 at 18:28 +0200, Hannes Reinecke wrote:
> Well, maybe; however, the current logic fails to match the entry
> 
> 	{"HITACHI", "OPEN-", "*", BLIST_REPORTLUN2},
> 
> against the 'real' name, which is "HITACHI" "OPEN-V".
> And for some reason we have far more customer using Hitachi arrays than
> using scanner of dubious provenance with no Vendor (which is the real
> bug if you ask me...)

Hello Hannes,

So for some entries in the table a prefix match should be performed (e.g. the
model string for the Hitachi entry) but for other entries (e.g. the scanner
entry) an exact match should be performed on the vendor name? If so, how about
one of the following two approaches:
* Adding ".*" at the end of the entries in the table for which a prefix match
  is sufficient and to change scsi_dev_info_list_find() such that it performs
  a regex match instead of an exact match or a prefix match.
* Performing an exact match on the vendor name and a prefix match on the model
  name.

Thanks,

Bart.

Re: [PATCH] scsi_devinfo: fixup string compare

[Hannes Reinecke]

On 08/04/2017 06:38 PM, Bart Van Assche wrote:
> On Fri, 2017-08-04 at 18:28 +0200, Hannes Reinecke wrote:
>> Well, maybe; however, the current logic fails to match the entry
>>
>> 	{"HITACHI", "OPEN-", "*", BLIST_REPORTLUN2},
>>
>> against the 'real' name, which is "HITACHI" "OPEN-V".
>> And for some reason we have far more customer using Hitachi arrays than
>> using scanner of dubious provenance with no Vendor (which is the real
>> bug if you ask me...)
> 
> Hello Hannes,
> 
> So for some entries in the table a prefix match should be performed (e.g. the
> model string for the Hitachi entry) but for other entries (e.g. the scanner
> entry) an exact match should be performed on the vendor name? If so, how about
> one of the following two approaches:
> * Adding ".*" at the end of the entries in the table for which a prefix match
>   is sufficient and to change scsi_dev_info_list_find() such that it performs
>   a regex match instead of an exact match or a prefix match.
> * Performing an exact match on the vendor name and a prefix match on the model
>   name.
> 
Doesn't work, as we have no idea which strings should be treated as
exact match and for which a partial match is applicable.

The solution is actually far simpler:
- Always assume partial match
- Check the min string length for vendor and model
- Do not attempt to match if the min length is zero

These rules drastically simplify the code.

Additionally I've done patches to export the blacklist value to sysfs,
_and_ modified scsi_debug to allow us to pass in the vendor and model
string.
With these modifications we can easily write blocktests to cover the
various corner cases, and have proper regression tests in case someone
needs to twiddle with that again.

Patchset to follow.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)