[PATCH] mdadm: replace hard coded string length

[PATCH] mdadm: replace hard coded string length

[Song Liu]

This patch replaces hard coded 32 with sizeof(sb->set_name) in a
couple places.

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 super1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/super1.c b/super1.c
index 9f62d23..7d03b1f 100644
--- a/super1.c
+++ b/super1.c
@@ -1030,7 +1030,7 @@ static void getinfo_super1(struct supertype *st, struct mdinfo *info, char *map)
 
 	memcpy(info->uuid, sb->set_uuid, 16);
 
-	strncpy(info->name, sb->set_name, 32);
+	strncpy(info->name, sb->set_name, sizeof(sb->set_name));
 	info->name[32] = 0;
 
 	if ((__le32_to_cpu(sb->feature_map)&MD_FEATURE_REPLACEMENT)) {
@@ -1124,7 +1124,7 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 		if (c)
 			strncpy(info->name, c+1, 31 - (c-sb->set_name));
 		else
-			strncpy(info->name, sb->set_name, 32);
+			strncpy(info->name, sb->set_name, sizeof(sb->set_name));
 		info->name[32] = 0;
 	}
 
-- 
2.8.0.rc2

Re: [PATCH] mdadm: replace hard coded string length

[Jes Sorensen]

Song Liu <songliubraving@fb.com> writes:
> This patch replaces hard coded 32 with sizeof(sb->set_name) in a
> couple places.
>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  super1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/super1.c b/super1.c
> index 9f62d23..7d03b1f 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1030,7 +1030,7 @@ static void getinfo_super1(struct supertype *st, struct mdinfo *info, char *map)
>  
>  	memcpy(info->uuid, sb->set_uuid, 16);
>  
> -	strncpy(info->name, sb->set_name, 32);
> +	strncpy(info->name, sb->set_name, sizeof(sb->set_name));
>  	info->name[32] = 0;
>  
>  	if ((__le32_to_cpu(sb->feature_map)&MD_FEATURE_REPLACEMENT)) {
> @@ -1124,7 +1124,7 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>  		if (c)
>  			strncpy(info->name, c+1, 31 - (c-sb->set_name));
>  		else
> -			strncpy(info->name, sb->set_name, 32);
> +			strncpy(info->name, sb->set_name, sizeof(sb->set_name));
>  		info->name[32] = 0;
>  	}

I was about to apply this, but this is actually wrong. You need to use
the size of the destination, not of the source as the limit.

Sorry for the hassle.

Jes

Re: [PATCH] mdadm: replace hard coded string length

[Thomas Fjellstrom]

On Thursday, September 15, 2016 12:15:30 PM MDT Jes Sorensen wrote:
> Song Liu <songliubraving@fb.com> writes:
> > This patch replaces hard coded 32 with sizeof(sb->set_name) in a
> > couple places.
> > 
> > Signed-off-by: Song Liu <songliubraving@fb.com>
> > ---
> > 
> >  super1.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/super1.c b/super1.c
> > index 9f62d23..7d03b1f 100644
> > --- a/super1.c
> > +++ b/super1.c
> > @@ -1030,7 +1030,7 @@ static void getinfo_super1(struct supertype *st,
> > struct mdinfo *info, char *map)> 
> >  	memcpy(info->uuid, sb->set_uuid, 16);
> > 
> > -	strncpy(info->name, sb->set_name, 32);
> > +	strncpy(info->name, sb->set_name, sizeof(sb->set_name));
> > 
> >  	info->name[32] = 0;
> >  	
> >  	if ((__le32_to_cpu(sb->feature_map)&MD_FEATURE_REPLACEMENT)) {
> > 
> > @@ -1124,7 +1124,7 @@ static int update_super1(struct supertype *st,
> > struct mdinfo *info,> 
> >  		if (c)
> >  		
> >  			strncpy(info->name, c+1, 31 - (c-sb->set_name));
> >  		
> >  		else
> > 
> > -			strncpy(info->name, sb->set_name, 32);
> > +			strncpy(info->name, sb->set_name, sizeof(sb->set_name));
> > 
> >  		info->name[32] = 0;
> >  	
> >  	}
> 
> I was about to apply this, but this is actually wrong. You need to use
> the size of the destination, not of the source as the limit.
> 
> Sorry for the hassle.

I'm not aware of the full details, but either they are the same size, or they 
aren't, and you need to use the minimum size of both to avoid any kind of 
overflow (source or dest, read and write). I presume the destination is 
smaller?

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


-- 
Thomas Fjellstrom
thomas@fjellstrom.ca

Re: [PATCH] mdadm: replace hard coded string length

[Adam Goryachev]

On 16/09/16 04:10, Thomas Fjellstrom wrote:
> On Thursday, September 15, 2016 12:15:30 PM MDT Jes Sorensen wrote:
>> Song Liu <songliubraving@fb.com> writes:
>>> This patch replaces hard coded 32 with sizeof(sb->set_name) in a
>>> couple places.
>>>
>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>> ---
>>>
>>>   super1.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/super1.c b/super1.c
>>> index 9f62d23..7d03b1f 100644
>>> --- a/super1.c
>>> +++ b/super1.c
>>> @@ -1030,7 +1030,7 @@ static void getinfo_super1(struct supertype *st,
>>> struct mdinfo *info, char *map)>
>>>   	memcpy(info->uuid, sb->set_uuid, 16);
>>>
>>> -	strncpy(info->name, sb->set_name, 32);
>>> +	strncpy(info->name, sb->set_name, sizeof(sb->set_name));
>>>
>>>   	info->name[32] = 0;
>>>   	
>>>   	if ((__le32_to_cpu(sb->feature_map)&MD_FEATURE_REPLACEMENT)) {
>>>
>>> @@ -1124,7 +1124,7 @@ static int update_super1(struct supertype *st,
>>> struct mdinfo *info,>
>>>   		if (c)
>>>   		
>>>   			strncpy(info->name, c+1, 31 - (c-sb->set_name));
>>>   		
>>>   		else
>>>
>>> -			strncpy(info->name, sb->set_name, 32);
>>> +			strncpy(info->name, sb->set_name, sizeof(sb->set_name));
>>>
>>>   		info->name[32] = 0;
>>>   	
>>>   	}
>> I was about to apply this, but this is actually wrong. You need to use
>> the size of the destination, not of the source as the limit.
>>
>> Sorry for the hassle.
> I'm not aware of the full details, but either they are the same size, or they
> aren't, and you need to use the minimum size of both to avoid any kind of
> overflow (source or dest, read and write). I presume the destination is
> smaller?
I'm not a programmer, but I think the size of the source is irrelevant. 
If the source is 10 bytes, you can safely copy that to a destination of 
30 bytes. The only problem is if the source content is bigger than the 
destination. Hence, you should copy only based on the destination size.

I'm not sure, but it may be a good idea to confirm that all of the 
source content has been copied, or else there may be unexpected results 
when operating on a truncated value.
I'm sure someone else who is an actual programmer can jump in and advise...

Regards,
Adam


-- 
Adam Goryachev Website Managers www.websitemanagers.com.au

Re: [PATCH] mdadm: replace hard coded string length

[Jes Sorensen]

Thomas Fjellstrom <thomas@fjellstrom.ca> writes:
> On Thursday, September 15, 2016 12:15:30 PM MDT Jes Sorensen wrote:
>> Song Liu <songliubraving@fb.com> writes:
>> > @@ -1124,7 +1124,7 @@ static int update_super1(struct supertype *st,
>> > struct mdinfo *info,> 
>> >  		if (c)
>> >  		
>> >  			strncpy(info->name, c+1, 31 - (c-sb->set_name));
>> >  		
>> >  		else
>> > 
>> > -			strncpy(info->name, sb->set_name, 32);
>> > +			strncpy(info->name, sb->set_name, sizeof(sb->set_name));
>> > 
>> >  		info->name[32] = 0;
>> >  	
>> >  	}
>> 
>> I was about to apply this, but this is actually wrong. You need to use
>> the size of the destination, not of the source as the limit.
>> 
>> Sorry for the hassle.
>
> I'm not aware of the full details, but either they are the same size, or they 
> aren't, and you need to use the minimum size of both to avoid any kind of 
> overflow (source or dest, read and write). I presume the destination is 
> smaller?

When copying a null terminated string, you need to check against the
size of the destination, not the source. It may happen to be they are
the same size here, but if code is later moved around you could get into
a situation where that is no longer the case. Checking against the size
of the destination is the correct way.

Second, when you reply to a mailing list posting, kindly refrain from
removing the person you respond to from the CC list.

Jes

Re: [PATCH] mdadm: replace hard coded string length

[Jes Sorensen]

Adam Goryachev <mailinglists@websitemanagers.com.au> writes:
> On 16/09/16 04:10, Thomas Fjellstrom wrote:
>> On Thursday, September 15, 2016 12:15:30 PM MDT Jes Sorensen wrote:
>>> I was about to apply this, but this is actually wrong. You need to use
>>> the size of the destination, not of the source as the limit.
>>>
>>> Sorry for the hassle.
>> I'm not aware of the full details, but either they are the same size, or they
>> aren't, and you need to use the minimum size of both to avoid any kind of
>> overflow (source or dest, read and write). I presume the destination is
>> smaller?
> I'm not a programmer, but I think the size of the source is
> irrelevant. If the source is 10 bytes, you can safely copy that to a
> destination of 30 bytes. The only problem is if the source content is
> bigger than the destination. Hence, you should copy only based on the
> destination size.
>
> I'm not sure, but it may be a good idea to confirm that all of the
> source content has been copied, or else there may be unexpected
> results when operating on a truncated value.  I'm sure someone else
> who is an actual programmer can jump in and advise...

Technically you are correct. However if we start adding such checks,
there's a full time job running through the code fixing up cases like
these. This for cases where we know it won't go wrong. It would also be
a large amount of patch churn for no gain.

Cheers,
Jes

Re: [PATCH] mdadm: replace hard coded string length

[Thomas Fjellstrom]

On Friday, September 16, 2016 8:34:44 AM MDT Jes Sorensen wrote:
> Thomas Fjellstrom <thomas@fjellstrom.ca> writes:
> > On Thursday, September 15, 2016 12:15:30 PM MDT Jes Sorensen wrote:
> >> Song Liu <songliubraving@fb.com> writes:
> >> > @@ -1124,7 +1124,7 @@ static int update_super1(struct supertype *st,
> >> > struct mdinfo *info,>
> >> > 
> >> >  		if (c)
> >> >  		
> >> >  			strncpy(info->name, c+1, 31 - (c-sb->set_name));
> >> >  		
> >> >  		else
> >> > 
> >> > -			strncpy(info->name, sb->set_name, 32);
> >> > +			strncpy(info->name, sb->set_name, sizeof(sb->set_name));
> >> > 
> >> >  		info->name[32] = 0;
> >> >  	
> >> >  	}
> >> 
> >> I was about to apply this, but this is actually wrong. You need to use
> >> the size of the destination, not of the source as the limit.
> >> 
> >> Sorry for the hassle.
> > 
> > I'm not aware of the full details, but either they are the same size, or
> > they aren't, and you need to use the minimum size of both to avoid any
> > kind of overflow (source or dest, read and write). I presume the
> > destination is smaller?
> 
> When copying a null terminated string, you need to check against the
> size of the destination, not the source. It may happen to be they are
> the same size here, but if code is later moved around you could get into
> a situation where that is no longer the case. Checking against the size
> of the destination is the correct way.

Yes, I wasn't paying close enough attention, str*cpy does the check on the 
source length so yes, you want to check against just the destination length in 
this case. In essense, you're checking both and clamping to the minimum of 
either buffer.

> Second, when you reply to a mailing list posting, kindly refrain from
> removing the person you respond to from the CC list.

Sorry, I felt like it'd be spamming the two other people.

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


-- 
Thomas Fjellstrom
thomas@fjellstrom.ca