LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] locomo.c: convert strncpy(x, y, sizeof(x)) to strlcpy
@ 2008-03-06 13:08 Roel Kluin
  2008-03-06 21:44 ` H. Peter Anvin
  0 siblings, 1 reply; 4+ messages in thread
From: Roel Kluin @ 2008-03-06 13:08 UTC (permalink / raw)
  To: elf; +Cc: Linux-arm, lkml

This patch was not yet tested. Please confirm it's right.
---
strncpy does not append '\0' if the length of the source string equals
the size parameter, strlcpy does.

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/arch/arm/common/locomo.c b/arch/arm/common/locomo.c
index ae21755..89b2328 100644
--- a/arch/arm/common/locomo.c
+++ b/arch/arm/common/locomo.c
@@ -516,7 +516,7 @@ locomo_init_one_child(struct locomo *lchip, struct locomo_dev_info *info)
 		goto out;
 	}
 
-	strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
+	strlcpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
 	/*
 	 * If the parent device has a DMA mask associated with it,
 	 * propagate it down to the children.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] locomo.c: convert strncpy(x, y, sizeof(x)) to strlcpy
  2008-03-06 13:08 [PATCH] locomo.c: convert strncpy(x, y, sizeof(x)) to strlcpy Roel Kluin
@ 2008-03-06 21:44 ` H. Peter Anvin
  2008-03-06 23:35   ` Roel Kluin
  0 siblings, 1 reply; 4+ messages in thread
From: H. Peter Anvin @ 2008-03-06 21:44 UTC (permalink / raw)
  To: Roel Kluin; +Cc: elf, Linux-arm, lkml

Roel Kluin wrote:
> This patch was not yet tested. Please confirm it's right.
> ---
> strncpy does not append '\0' if the length of the source string equals
> the size parameter, strlcpy does.
> 

Are you sure it's safe to not zero out the contents of the buffer (no 
information leak)?

	-hpa

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] locomo.c: convert strncpy(x, y, sizeof(x)) to strlcpy
  2008-03-06 21:44 ` H. Peter Anvin
@ 2008-03-06 23:35   ` Roel Kluin
  2008-03-07  1:51     ` David Wagner
  0 siblings, 1 reply; 4+ messages in thread
From: Roel Kluin @ 2008-03-06 23:35 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: elf, Linux-arm, lkml

H. Peter Anvin wrote:
> Roel Kluin wrote:
>> This patch was not yet tested. Please confirm it's right.
>> ---
>> strncpy does not append '\0' if the length of the source string equals
>> the size parameter, strlcpy does.
>>
> 
> Are you sure it's safe to not zero out the contents of the buffer (no
> information leak)?
> 
>     -hpa

As I understand it, please correct me if I'm wrong:

Of the three variants: strcpy, strncpy and strlcpy.
- strcpy does not append \0 (unless the source string already contained it)
- strncpy appends \0's if the source string is smaller than the size
  parameter (for all remaining characters)
- strlcpy always appends a single \0 (unless size parameter was 0)

char *strcpy(char *dest, const char *src);
char *strncpy(char *dest, const char *src, size_t n);
size_t strlcpy(char *dst, const char *src, size_t n);

In the original code strncpy was used and the size parameter was equal
to the source string size:

strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));

Since this the size was equal there was no \0 termination. To \0
terminate using strncpy we could write:

strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id) - 1);
dev->dev.bus_id[sizeof(dev->dev.bus_id) - 1] = '\0';

or using strlcpy, which does the same thing:

strlcpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));

Roel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] locomo.c: convert strncpy(x, y, sizeof(x)) to strlcpy
  2008-03-06 23:35   ` Roel Kluin
@ 2008-03-07  1:51     ` David Wagner
  0 siblings, 0 replies; 4+ messages in thread
From: David Wagner @ 2008-03-07  1:51 UTC (permalink / raw)
  To: linux-kernel

Roel Kluin  wrote:
>As I understand it, please correct me if I'm wrong:
>
>Of the three variants: strcpy, strncpy and strlcpy.
>- strcpy does not append \0 (unless the source string already contained it)

No, that's not correct.  If the source string has no \0, then
strcpy keeps reading forever and things go horribly awry.

>In the original code strncpy was used and the size parameter was equal
>to the source string size:
>
>strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
>
>Since this the size was equal there was no \0 termination. To \0
>terminate using strncpy we could write:
>
>strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id) - 1);
>dev->dev.bus_id[sizeof(dev->dev.bus_id) - 1] = '\0';
>
>or using strlcpy, which does the same thing:
>
>strlcpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));

No, strlcpy() does not do the same thing.  As several people have pointed
out to you, strlcpy() does not fill the rest of the buffer with \0's.
This can create an information leak in some cases, which can create a
security hole.  The difference is important in some cases.

Second, you seem to implicitly assume that it's a bug to leave
dev->dev.bus_id without any \0 termination.  You'd have to look at the API
to determine whether that assumption is accurate.  And then you'd have to
think carefully about what is the proper behavior if info->name is too
long to fit into the buffer.  Is truncating the string the appropriate
behavior?  In some cases prematurely truncating a string is bad news and
can create a security bug, and the proper thing to do in some cases may
be to return an error rather than returning a truncated string.

Rant: Blindly search-and-replacing strncpy() with strlcpy() is not safe,
in general.  You have to actually understand what the code is doing and
what it should be doing.

I'm not claiming that your patch is correct, nor that it is incorrect.
Instead, I'm suggesting that you probably need to do some analysis to
understand the specific requirements and context in each of these cases,
rather than doing a mechanically search-and-replace.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2008-03-07  2:50 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-06 13:08 [PATCH] locomo.c: convert strncpy(x, y, sizeof(x)) to strlcpy Roel Kluin
2008-03-06 21:44 ` H. Peter Anvin
2008-03-06 23:35   ` Roel Kluin
2008-03-07  1:51     ` David Wagner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).