[PATCH infiniband-diags 2/6] ibstat.c: fix buffer-not-null-terminated

[PATCH infiniband-diags 2/6] ibstat.c: fix buffer-not-null-terminated

[Hal Rosenstock]

From: Dan Ben Yosef <danby-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>

Buffer may not have a null terminator if the source string's length is
equal to the buffer size.

Signed-off-by: Dan Ben Yosef <danby-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 src/ibstat.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/ibstat.c b/src/ibstat.c
index 665bb0a..acb9e8e 100644
--- a/src/ibstat.c
+++ b/src/ibstat.c
@@ -279,6 +279,7 @@ int main(int argc, char *argv[])
 	int dev_port = -1;
 	int n, i;
 
+	memset(names, 0, sizeof(names[0][0] * UMAD_MAX_DEVICES * UMAD_CA_NAME_LEN));
 	const struct ibdiag_opt opts[] = {
 		{"list_of_cas", 'l', 0, NULL, "list all IB devices"},
 		{"short", 's', 0, NULL, "short output"},
@@ -314,7 +315,7 @@ int main(int argc, char *argv[])
 		if (i >= n)
 			IBPANIC("'%s' IB device can't be found", argv[0]);
 
-		strncpy(names[i], argv[0], sizeof names[i]);
+		strncpy(names[i], argv[0], sizeof names[i]-1);
 		n = 1;
 	}
 
-- 
1.7.8.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH infiniband-diags 2/6] ibstat.c: fix buffer-not-null-terminated

[Ira Weiny]

On Wed, 12 Jun 2013 09:55:32 -0400
Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:

> From: Dan Ben Yosef <danby-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
> 
> Buffer may not have a null terminator if the source string's length is
> equal to the buffer size.

Good catch, but I think this is an issue in umad_get_cas_names!

> 
> Signed-off-by: Dan Ben Yosef <danby-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  src/ibstat.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/src/ibstat.c b/src/ibstat.c
> index 665bb0a..acb9e8e 100644
> --- a/src/ibstat.c
> +++ b/src/ibstat.c
> @@ -279,6 +279,7 @@ int main(int argc, char *argv[])
>  	int dev_port = -1;
>  	int n, i;
>  
> +	memset(names, 0, sizeof(names[0][0] * UMAD_MAX_DEVICES * UMAD_CA_NAME_LEN));

I don't think you need this as it will not fix umad_get_cas_names.

>  	const struct ibdiag_opt opts[] = {
>  		{"list_of_cas", 'l', 0, NULL, "list all IB devices"},
>  		{"short", 's', 0, NULL, "short output"},
> @@ -314,7 +315,7 @@ int main(int argc, char *argv[])
>  		if (i >= n)
>  			IBPANIC("'%s' IB device can't be found", argv[0]);
>  
> -		strncpy(names[i], argv[0], sizeof names[i]);
> +		strncpy(names[i], argv[0], sizeof names[i]-1);

This is actually dead code.  IBPANIC exits, if your linking to libibmad.  Do you have a different IBPANIC which does not exit?  Do you have a use case which hits this bug?

[root@iqa-136 sbin]# ./ibstat myverylongca_name_0123456789_0123456789
ibpanic: [9262] main: 'myverylongca_name_0123456789_0123456789' IB device can't be found: Success
[root@iqa-136 sbin]# ./ibstat notfoundca
ibpanic: [9273] main: 'notfoundca' IB device can't be found: Success

Ira

>  		n = 1;
>  	}
>  
> -- 
> 1.7.8.2
> 


-- 
Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH infiniband-diags 2/6] ibstat.c: fix buffer-not-null-terminated

[Hal Rosenstock]

On 6/12/2013 5:19 PM, Ira Weiny wrote:
> On Wed, 12 Jun 2013 09:55:32 -0400
> Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> 
>> From: Dan Ben Yosef <danby-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
>>
>> Buffer may not have a null terminator if the source string's length is
>> equal to the buffer size.
> 
> Good catch, but I think this is an issue in umad_get_cas_names!
> 
>>
>> Signed-off-by: Dan Ben Yosef <danby-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
>> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> ---
>>  src/ibstat.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/ibstat.c b/src/ibstat.c
>> index 665bb0a..acb9e8e 100644
>> --- a/src/ibstat.c
>> +++ b/src/ibstat.c
>> @@ -279,6 +279,7 @@ int main(int argc, char *argv[])
>>  	int dev_port = -1;
>>  	int n, i;
>>  
>> +	memset(names, 0, sizeof(names[0][0] * UMAD_MAX_DEVICES * UMAD_CA_NAME_LEN));
> 
> I don't think you need this as it will not fix umad_get_cas_names.

The proper memset is already in the tree.

>>  	const struct ibdiag_opt opts[] = {
>>  		{"list_of_cas", 'l', 0, NULL, "list all IB devices"},
>>  		{"short", 's', 0, NULL, "short output"},
>> @@ -314,7 +315,7 @@ int main(int argc, char *argv[])
>>  		if (i >= n)
>>  			IBPANIC("'%s' IB device can't be found", argv[0]);
>>  
>> -		strncpy(names[i], argv[0], sizeof names[i]);
>> +		strncpy(names[i], argv[0], sizeof names[i]-1);
> 
> This is actually dead code.  IBPANIC exits, if your linking to libibmad.  Do you have a different IBPANIC which does not exit?  Do you have a use case which hits this bug?
> 
> [root@iqa-136 sbin]# ./ibstat myverylongca_name_0123456789_0123456789
> ibpanic: [9262] main: 'myverylongca_name_0123456789_0123456789' IB device can't be found: Success
> [root@iqa-136 sbin]# ./ibstat notfoundca
> ibpanic: [9273] main: 'notfoundca' IB device can't be found: Success

I think the change is to quiet a Coverity detected error but Dan is the
definitive source for this change.

-- Hal

> Ira
> 
>>  		n = 1;
>>  	}
>>  
>> -- 
>> 1.7.8.2
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH infiniband-diags 2/6] ibstat.c: fix buffer-not-null-terminated

[Hal Rosenstock]

On 6/12/2013 6:03 PM, Hal Rosenstock wrote:
> On 6/12/2013 5:19 PM, Ira Weiny wrote:
>> On Wed, 12 Jun 2013 09:55:32 -0400
>> Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
>>
>>> From: Dan Ben Yosef <danby-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
>>>
>>> Buffer may not have a null terminator if the source string's length is
>>> equal to the buffer size.
>>
>> Good catch, but I think this is an issue in umad_get_cas_names!
>>
>>>
>>> Signed-off-by: Dan Ben Yosef <danby-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
>>> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>> ---
>>>  src/ibstat.c |    3 ++-
>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/src/ibstat.c b/src/ibstat.c
>>> index 665bb0a..acb9e8e 100644
>>> --- a/src/ibstat.c
>>> +++ b/src/ibstat.c
>>> @@ -279,6 +279,7 @@ int main(int argc, char *argv[])
>>>  	int dev_port = -1;
>>>  	int n, i;
>>>  
>>> +	memset(names, 0, sizeof(names[0][0] * UMAD_MAX_DEVICES * UMAD_CA_NAME_LEN));
>>
>> I don't think you need this as it will not fix umad_get_cas_names.
> 
> The proper memset is already in the tree.

It isn't in the tree; I was looking at modified source...

>>>  	const struct ibdiag_opt opts[] = {
>>>  		{"list_of_cas", 'l', 0, NULL, "list all IB devices"},
>>>  		{"short", 's', 0, NULL, "short output"},
>>> @@ -314,7 +315,7 @@ int main(int argc, char *argv[])
>>>  		if (i >= n)
>>>  			IBPANIC("'%s' IB device can't be found", argv[0]);
>>>  
>>> -		strncpy(names[i], argv[0], sizeof names[i]);
>>> +		strncpy(names[i], argv[0], sizeof names[i]-1);
>>
>> This is actually dead code.  IBPANIC exits, if your linking to libibmad.  Do you have a different IBPANIC which does not exit?  Do you have a use case which hits this bug?
>>
>> [root@iqa-136 sbin]# ./ibstat myverylongca_name_0123456789_0123456789
>> ibpanic: [9262] main: 'myverylongca_name_0123456789_0123456789' IB device can't be found: Success
>> [root@iqa-136 sbin]# ./ibstat notfoundca
>> ibpanic: [9273] main: 'notfoundca' IB device can't be found: Success
> 
> I think the change is to quiet a Coverity detected error but Dan is the
> definitive source for this change.
> 
> -- Hal
> 
>> Ira
>>
>>>  		n = 1;
>>>  	}
>>>  
>>> -- 
>>> 1.7.8.2
>>>
>>
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH infiniband-diags 2/6] ibstat.c: fix buffer-not-null-terminated

[Weiny, Ira]

> -----Original Message-----
> From: Hal Rosenstock [mailto:hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org]
> Subject: Re: [PATCH infiniband-diags 2/6] ibstat.c: fix buffer-not-null-
> terminated
> 
> On 6/12/2013 6:03 PM, Hal Rosenstock wrote:
> > On 6/12/2013 5:19 PM, Ira Weiny wrote:
> >> On Wed, 12 Jun 2013 09:55:32 -0400
> >> Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> >>
> >>> From: Dan Ben Yosef <danby-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
> >>>
> >>>
> >>> +	memset(names, 0, sizeof(names[0][0] * UMAD_MAX_DEVICES *
> >>> +UMAD_CA_NAME_LEN));
> >>
> >> I don't think you need this as it will not fix umad_get_cas_names.
> >
> > The proper memset is already in the tree.
> 
> It isn't in the tree; I was looking at modified source...

Sounds like that should be pushed upstream!  ;-)

> 
> >>>  	const struct ibdiag_opt opts[] = {
> >>>  		{"list_of_cas", 'l', 0, NULL, "list all IB devices"},
> >>>  		{"short", 's', 0, NULL, "short output"}, @@ -314,7 +315,7 @@
> int
> >>> main(int argc, char *argv[])
> >>>  		if (i >= n)
> >>>  			IBPANIC("'%s' IB device can't be found", argv[0]);
> >>>
> >>> -		strncpy(names[i], argv[0], sizeof names[i]);
> >>> +		strncpy(names[i], argv[0], sizeof names[i]-1);
> >>
> >> This is actually dead code.  IBPANIC exits, if your linking to libibmad.  Do
> you have a different IBPANIC which does not exit?  Do you have a use case
> which hits this bug?
> >>
> >> [root@iqa-136 sbin]# ./ibstat
> myverylongca_name_0123456789_0123456789
> >> ibpanic: [9262] main: 'myverylongca_name_0123456789_0123456789' IB
> >> device can't be found: Success
> >> [root@iqa-136 sbin]# ./ibstat notfoundca
> >> ibpanic: [9273] main: 'notfoundca' IB device can't be found: Success
> >
> > I think the change is to quiet a Coverity detected error but Dan is
> > the definitive source for this change.

I think we should remove the dead code and that should fix Coverity.  I thought it detected dead code.  I wonder if I missed something?

Ira

> >
> > -- Hal
> >
> >> Ira
> >>
> >>>  		n = 1;
> >>>  	}
> >>>
> >>> --
> >>> 1.7.8.2
> >>>
> >>
> >>
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH infiniband-diags 2/6] ibstat.c: fix buffer-not-null-terminated

[Weiny, Ira]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> Subject: RE: [PATCH infiniband-diags 2/6] ibstat.c: fix buffer-not-null-
> terminated
> 
> > >
> > > I think the change is to quiet a Coverity detected error but Dan is
> > > the definitive source for this change.
> 
> I think we should remove the dead code and that should fix Coverity.  I
> thought it detected dead code.  I wonder if I missed something?

That is not dead code.  I misread where the break jumps to.
	
However, the code is redundant and there is a bug.[*]

16:16:51 > ibstat -l
mlx4_0
mlx4_1
16:17:57 > ibstat -l mlx4_1
mlx4_0

The intent was to copy argv[0] into names[0] and set n=1.  But that is not happening...
	
Ira

[*] not that the above is really a valid use case.
	
> 
> Ira
> 
> > >
> > > -- Hal
> > >
> > >> Ira
> > >>
> > >>>  		n = 1;
> > >>>  	}
> > >>>
> > >>> --
> > >>> 1.7.8.2
> > >>>
> > >>
> > >>
> > >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the
> body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html