[PATCH] nvme: fix string_matches() helper

[PATCH] nvme: fix string_matches() helper

[Christoph Hellwig]

NVMe "ASCII" strings are not nul-terminated and can use up every single
byte in the field.  Thus use strnlen to determine the match length instead
of possibly overrunning the field.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 434b18863895..2658a3a3cbb5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1581,9 +1581,7 @@ static bool string_matches(const char *idstr, const char *match, size_t len)
 	if (!match)
 		return true;
 
-	matchlen = strlen(match);
-	WARN_ON_ONCE(matchlen > len);
-
+	matchlen = strnlen(match, len);
 	if (memcmp(idstr, match, matchlen))
 		return false;
 
-- 
2.11.0

[PATCH] nvme: fix string_matches() helper

[Andy Lutomirski]

On Tue, Jun 13, 2017@6:07 AM, Christoph Hellwig <hch@lst.de> wrote:
> NVMe "ASCII" strings are not nul-terminated and can use up every single
> byte in the field.  Thus use strnlen to determine the match length instead
> of possibly overrunning the field.
>
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>  drivers/nvme/host/core.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 434b18863895..2658a3a3cbb5 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1581,9 +1581,7 @@ static bool string_matches(const char *idstr, const char *match, size_t len)
>         if (!match)
>                 return true;
>
> -       matchlen = strlen(match);
> -       WARN_ON_ONCE(matchlen > len);
> -
> +       matchlen = strnlen(match, len);

"match" refers to the string in the quirk table, which should be a
plain C string.  Are you hitting this in practice?

[PATCH] nvme: fix string_matches() helper

[Christoph Hellwig]

On Tue, Jun 13, 2017@08:42:59AM -0700, Andy Lutomirski wrote:
> On Tue, Jun 13, 2017@6:07 AM, Christoph Hellwig <hch@lst.de> wrote:
> > NVMe "ASCII" strings are not nul-terminated and can use up every single
> > byte in the field.  Thus use strnlen to determine the match length instead
> > of possibly overrunning the field.
> >
> > Signed-off-by: Christoph Hellwig <hch at lst.de>
> > ---
> >  drivers/nvme/host/core.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 434b18863895..2658a3a3cbb5 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -1581,9 +1581,7 @@ static bool string_matches(const char *idstr, const char *match, size_t len)
> >         if (!match)
> >                 return true;
> >
> > -       matchlen = strlen(match);
> > -       WARN_ON_ONCE(matchlen > len);
> > -
> > +       matchlen = strnlen(match, len);
> 
> "match" refers to the string in the quirk table, which should be a
> plain C string.  Are you hitting this in practice?

No, but I've just written some other code that deals with NVMe
strings, and took extra care of bounds protection.

But yes, given the strlen is on the match we should be ok as long
as the WARN_ON_ONCE also does an early return.  Does that sound ok
to you?

[PATCH] nvme: fix string_matches() helper

[Sagi Grimberg]

This looks fine to me,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me

[PATCH] nvme: fix string_matches() helper

[Andy Lutomirski]

On Tue, Jun 13, 2017@11:38 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, Jun 13, 2017@08:42:59AM -0700, Andy Lutomirski wrote:
>> On Tue, Jun 13, 2017@6:07 AM, Christoph Hellwig <hch@lst.de> wrote:
>> > NVMe "ASCII" strings are not nul-terminated and can use up every single
>> > byte in the field.  Thus use strnlen to determine the match length instead
>> > of possibly overrunning the field.
>> >
>> > Signed-off-by: Christoph Hellwig <hch at lst.de>
>> > ---
>> >  drivers/nvme/host/core.c | 4 +---
>> >  1 file changed, 1 insertion(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> > index 434b18863895..2658a3a3cbb5 100644
>> > --- a/drivers/nvme/host/core.c
>> > +++ b/drivers/nvme/host/core.c
>> > @@ -1581,9 +1581,7 @@ static bool string_matches(const char *idstr, const char *match, size_t len)
>> >         if (!match)
>> >                 return true;
>> >
>> > -       matchlen = strlen(match);
>> > -       WARN_ON_ONCE(matchlen > len);
>> > -
>> > +       matchlen = strnlen(match, len);
>>
>> "match" refers to the string in the quirk table, which should be a
>> plain C string.  Are you hitting this in practice?
>
> No, but I've just written some other code that deals with NVMe
> strings, and took extra care of bounds protection.
>
> But yes, given the strlen is on the match we should be ok as long
> as the WARN_ON_ONCE also does an early return.  Does that sound ok
> to you?

Sounds good to me.