LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Make v9fs uname and remotename parsing more robust
@ 2008-02-15 17:54 Markus Armbruster
  2008-02-22 15:57 ` [PATCH] 9p: Make " Markus Armbruster
  2008-02-23  8:07 ` [PATCH] Make v9fs " Andrew Morton
  0 siblings, 2 replies; 5+ messages in thread
From: Markus Armbruster @ 2008-02-15 17:54 UTC (permalink / raw)
  To: linux-kernel; +Cc: Latchesar Ionkov, Eric Van Hensbergen, Jim Meyering

match_strcpy() is a somewhat creepy function: the caller needs to make
sure that the destination buffer is big enough, and when he screws up
or forgets, match_strcpy() happily overruns the buffer.

There's exactly one customer: v9fs_parse_options().  I believe it
currently can't overflow its buffer, but that's not exactly obvious.

The source string is a substing of the mount options.  The kernel
silently truncates those to PAGE_SIZE bytes, including the terminating
zero.  See compat_sys_mount() and do_mount().

The destination buffer is obtained from __getname(), which allocates
from name_cachep, which is initialized by vfs_caches_init() for size
PATH_MAX.

We're safe as long as PATH_MAX <= PAGE_SIZE.  PATH_MAX is 4096.  As
far as I know, the smallest PAGE_SIZE is also 4096.

Here's a patch that makes the code a bit more obviously correct.  It
doesn't depend on PATH_MAX <= PAGE_SIZE.

Signed-off-by: Markus Armbruster <armbru@redhat.com>


diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
index fbb12da..e42948b 100644
--- a/fs/9p/v9fs.c
+++ b/fs/9p/v9fs.c
@@ -135,10 +135,10 @@ static void v9fs_parse_options(struct v9fs_session_info *v9ses)
 			v9ses->trans = v9fs_match_trans(&args[0]);
 			break;
 		case Opt_uname:
-			match_strcpy(v9ses->uname, &args[0]);
+			match_strlcpy(v9ses->uname, &args[0], PATH_MAX);
 			break;
 		case Opt_remotename:
-			match_strcpy(v9ses->aname, &args[0]);
+			match_strlcpy(v9ses->aname, &args[0], PATH_MAX);
 			break;
 		case Opt_legacy:
 			v9ses->flags &= ~V9FS_EXTENDED;
diff --git a/include/linux/parser.h b/include/linux/parser.h
index 26b2bdf..7dcd050 100644
--- a/include/linux/parser.h
+++ b/include/linux/parser.h
@@ -29,5 +29,5 @@ int match_token(char *, match_table_t table, substring_t args[]);
 int match_int(substring_t *, int *result);
 int match_octal(substring_t *, int *result);
 int match_hex(substring_t *, int *result);
-void match_strcpy(char *, const substring_t *);
+size_t match_strlcpy(char *, const substring_t *, size_t);
 char *match_strdup(const substring_t *);
diff --git a/lib/parser.c b/lib/parser.c
index 703c8c1..4f0cbc0 100644
--- a/lib/parser.c
+++ b/lib/parser.c
@@ -182,18 +182,25 @@ int match_hex(substring_t *s, int *result)
 }
 
 /**
- * match_strcpy: - copies the characters from a substring_t to a string
- * @to: string to copy characters to.
- * @s: &substring_t to copy
+ * match_strlcpy: - Copy the characters from a substring_t to a sized buffer
+ * @dest: where to copy to
+ * @src: &substring_t to copy
+ * @size: size of destination buffer
  *
- * Description: Copies the set of characters represented by the given
- * &substring_t @s to the c-style string @to. Caller guarantees that @to is
- * large enough to hold the characters of @s.
+ * Description: Copy the characters in &substring_t @src to the
+ * c-style string @dest.  Copy no more than @size - 1 characters, plus
+ * the terminating NUL.  Return length of @src.
  */
-void match_strcpy(char *to, const substring_t *s)
+size_t match_strlcpy(char *dest, const substring_t *src, size_t size)
 {
-	memcpy(to, s->from, s->to - s->from);
-	to[s->to - s->from] = '\0';
+	size_t ret = src->to - src->from;
+
+	if (size) {
+		size_t len = ret >= size ? size - 1 : ret;
+		memcpy(dest, src->from, len);
+		dest[len] = '\0';
+	}
+	return ret;
 }
 
 /**
@@ -206,9 +213,10 @@ void match_strcpy(char *to, const substring_t *s)
  */
 char *match_strdup(const substring_t *s)
 {
-	char *p = kmalloc(s->to - s->from + 1, GFP_KERNEL);
+	size_t sz = s->to - s->from + 1;
+	char *p = kmalloc(sz, GFP_KERNEL);
 	if (p)
-		match_strcpy(p, s);
+		match_strlcpy(p, s, sz);
 	return p;
 }
 
@@ -216,5 +224,5 @@ EXPORT_SYMBOL(match_token);
 EXPORT_SYMBOL(match_int);
 EXPORT_SYMBOL(match_octal);
 EXPORT_SYMBOL(match_hex);
-EXPORT_SYMBOL(match_strcpy);
+EXPORT_SYMBOL(match_strlcpy);
 EXPORT_SYMBOL(match_strdup);

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

* [PATCH] 9p: Make uname and remotename parsing more robust
  2008-02-15 17:54 [PATCH] Make v9fs uname and remotename parsing more robust Markus Armbruster
@ 2008-02-22 15:57 ` Markus Armbruster
  2008-02-23  8:07 ` [PATCH] Make v9fs " Andrew Morton
  1 sibling, 0 replies; 5+ messages in thread
From: Markus Armbruster @ 2008-02-22 15:57 UTC (permalink / raw)
  To: linux-kernel; +Cc: Latchesar Ionkov, Eric Van Hensbergen, Jim Meyering

match_strcpy() is a somewhat creepy function: the caller needs to make
sure that the destination buffer is big enough, and when he screws up
or forgets, match_strcpy() happily overruns the buffer.

There's exactly one customer: v9fs_parse_options().  I believe it
currently can't overflow its buffer, but that's not exactly obvious.

The source string is a substring of the mount options.  The kernel
silently truncates those to PAGE_SIZE bytes, including the terminating
zero.  See compat_sys_mount() and do_mount().

The destination buffer is obtained from __getname(), which allocates
from name_cachep, which is initialized by vfs_caches_init() for size
PATH_MAX.

We're safe as long as PATH_MAX <= PAGE_SIZE.  PATH_MAX is 4096.  As
far as I know, the smallest PAGE_SIZE is also 4096.

Here's a patch that makes the code a bit more obviously correct.  It
doesn't depend on PATH_MAX <= PAGE_SIZE.

Signed-off-by: Markus Armbruster <armbru@redhat.com>


diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
index fbb12da..e42948b 100644
--- a/fs/9p/v9fs.c
+++ b/fs/9p/v9fs.c
@@ -135,10 +135,10 @@ static void v9fs_parse_options(struct v9fs_session_info *v9ses)
 			v9ses->trans = v9fs_match_trans(&args[0]);
 			break;
 		case Opt_uname:
-			match_strcpy(v9ses->uname, &args[0]);
+			match_strlcpy(v9ses->uname, &args[0], PATH_MAX);
 			break;
 		case Opt_remotename:
-			match_strcpy(v9ses->aname, &args[0]);
+			match_strlcpy(v9ses->aname, &args[0], PATH_MAX);
 			break;
 		case Opt_legacy:
 			v9ses->flags &= ~V9FS_EXTENDED;
diff --git a/include/linux/parser.h b/include/linux/parser.h
index 26b2bdf..7dcd050 100644
--- a/include/linux/parser.h
+++ b/include/linux/parser.h
@@ -29,5 +29,5 @@ int match_token(char *, match_table_t table, substring_t args[]);
 int match_int(substring_t *, int *result);
 int match_octal(substring_t *, int *result);
 int match_hex(substring_t *, int *result);
-void match_strcpy(char *, const substring_t *);
+size_t match_strlcpy(char *, const substring_t *, size_t);
 char *match_strdup(const substring_t *);
diff --git a/lib/parser.c b/lib/parser.c
index 703c8c1..4f0cbc0 100644
--- a/lib/parser.c
+++ b/lib/parser.c
@@ -182,18 +182,25 @@ int match_hex(substring_t *s, int *result)
 }
 
 /**
- * match_strcpy: - copies the characters from a substring_t to a string
- * @to: string to copy characters to.
- * @s: &substring_t to copy
+ * match_strlcpy: - Copy the characters from a substring_t to a sized buffer
+ * @dest: where to copy to
+ * @src: &substring_t to copy
+ * @size: size of destination buffer
  *
- * Description: Copies the set of characters represented by the given
- * &substring_t @s to the c-style string @to. Caller guarantees that @to is
- * large enough to hold the characters of @s.
+ * Description: Copy the characters in &substring_t @src to the
+ * c-style string @dest.  Copy no more than @size - 1 characters, plus
+ * the terminating NUL.  Return length of @src.
  */
-void match_strcpy(char *to, const substring_t *s)
+size_t match_strlcpy(char *dest, const substring_t *src, size_t size)
 {
-	memcpy(to, s->from, s->to - s->from);
-	to[s->to - s->from] = '\0';
+	size_t ret = src->to - src->from;
+
+	if (size) {
+		size_t len = ret >= size ? size - 1 : ret;
+		memcpy(dest, src->from, len);
+		dest[len] = '\0';
+	}
+	return ret;
 }
 
 /**
@@ -206,9 +213,10 @@ void match_strcpy(char *to, const substring_t *s)
  */
 char *match_strdup(const substring_t *s)
 {
-	char *p = kmalloc(s->to - s->from + 1, GFP_KERNEL);
+	size_t sz = s->to - s->from + 1;
+	char *p = kmalloc(sz, GFP_KERNEL);
 	if (p)
-		match_strcpy(p, s);
+		match_strlcpy(p, s, sz);
 	return p;
 }
 
@@ -216,5 +224,5 @@ EXPORT_SYMBOL(match_token);
 EXPORT_SYMBOL(match_int);
 EXPORT_SYMBOL(match_octal);
 EXPORT_SYMBOL(match_hex);
-EXPORT_SYMBOL(match_strcpy);
+EXPORT_SYMBOL(match_strlcpy);
 EXPORT_SYMBOL(match_strdup);

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

* Re: [PATCH] Make v9fs uname and remotename parsing more robust
  2008-02-15 17:54 [PATCH] Make v9fs uname and remotename parsing more robust Markus Armbruster
  2008-02-22 15:57 ` [PATCH] 9p: Make " Markus Armbruster
@ 2008-02-23  8:07 ` Andrew Morton
  2008-02-24 15:37   ` Eric Van Hensbergen
  1 sibling, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2008-02-23  8:07 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: linux-kernel, Latchesar Ionkov, Eric Van Hensbergen, Jim Meyering

On Fri, 15 Feb 2008 18:54:34 +0100 Markus Armbruster <armbru@redhat.com> wrote:

> match_strcpy() is a somewhat creepy function: the caller needs to make
> sure that the destination buffer is big enough, and when he screws up
> or forgets, match_strcpy() happily overruns the buffer.
> 
> There's exactly one customer: v9fs_parse_options().  I believe it
> currently can't overflow its buffer, but that's not exactly obvious.
> 
> The source string is a substing of the mount options.  The kernel
> silently truncates those to PAGE_SIZE bytes, including the terminating
> zero.  See compat_sys_mount() and do_mount().
> 
> The destination buffer is obtained from __getname(), which allocates
> from name_cachep, which is initialized by vfs_caches_init() for size
> PATH_MAX.
> 
> We're safe as long as PATH_MAX <= PAGE_SIZE.  PATH_MAX is 4096.  As
> far as I know, the smallest PAGE_SIZE is also 4096.
> 
> Here's a patch that makes the code a bit more obviously correct.  It
> doesn't depend on PATH_MAX <= PAGE_SIZE.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> 
> diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
> index fbb12da..e42948b 100644
> --- a/fs/9p/v9fs.c
> +++ b/fs/9p/v9fs.c
> @@ -135,10 +135,10 @@ static void v9fs_parse_options(struct v9fs_session_info *v9ses)
>  			v9ses->trans = v9fs_match_trans(&args[0]);
>  			break;
>  		case Opt_uname:
> -			match_strcpy(v9ses->uname, &args[0]);
> +			match_strlcpy(v9ses->uname, &args[0], PATH_MAX);
>  			break;
>  		case Opt_remotename:
> -			match_strcpy(v9ses->aname, &args[0]);
> +			match_strlcpy(v9ses->aname, &args[0], PATH_MAX);
>  			break;
>  		case Opt_legacy:
>  			v9ses->flags &= ~V9FS_EXTENDED;
> diff --git a/include/linux/parser.h b/include/linux/parser.h
> index 26b2bdf..7dcd050 100644
> --- a/include/linux/parser.h
> +++ b/include/linux/parser.h
> @@ -29,5 +29,5 @@ int match_token(char *, match_table_t table, substring_t args[]);
>  int match_int(substring_t *, int *result);
>  int match_octal(substring_t *, int *result);
>  int match_hex(substring_t *, int *result);
> -void match_strcpy(char *, const substring_t *);
> +size_t match_strlcpy(char *, const substring_t *, size_t);
>  char *match_strdup(const substring_t *);
> diff --git a/lib/parser.c b/lib/parser.c
> index 703c8c1..4f0cbc0 100644
> --- a/lib/parser.c
> +++ b/lib/parser.c
> @@ -182,18 +182,25 @@ int match_hex(substring_t *s, int *result)
>  }
>  
>  /**
> - * match_strcpy: - copies the characters from a substring_t to a string
> - * @to: string to copy characters to.
> - * @s: &substring_t to copy
> + * match_strlcpy: - Copy the characters from a substring_t to a sized buffer
> + * @dest: where to copy to
> + * @src: &substring_t to copy
> + * @size: size of destination buffer
>   *
> - * Description: Copies the set of characters represented by the given
> - * &substring_t @s to the c-style string @to. Caller guarantees that @to is
> - * large enough to hold the characters of @s.
> + * Description: Copy the characters in &substring_t @src to the
> + * c-style string @dest.  Copy no more than @size - 1 characters, plus
> + * the terminating NUL.  Return length of @src.
>   */
> -void match_strcpy(char *to, const substring_t *s)
> +size_t match_strlcpy(char *dest, const substring_t *src, size_t size)
>  {
> -	memcpy(to, s->from, s->to - s->from);
> -	to[s->to - s->from] = '\0';
> +	size_t ret = src->to - src->from;
> +
> +	if (size) {
> +		size_t len = ret >= size ? size - 1 : ret;
> +		memcpy(dest, src->from, len);
> +		dest[len] = '\0';
> +	}
> +	return ret;
>  }
>  
>  /**
> @@ -206,9 +213,10 @@ void match_strcpy(char *to, const substring_t *s)
>   */
>  char *match_strdup(const substring_t *s)
>  {
> -	char *p = kmalloc(s->to - s->from + 1, GFP_KERNEL);
> +	size_t sz = s->to - s->from + 1;
> +	char *p = kmalloc(sz, GFP_KERNEL);
>  	if (p)
> -		match_strcpy(p, s);
> +		match_strlcpy(p, s, sz);
>  	return p;
>  }
>  
> @@ -216,5 +224,5 @@ EXPORT_SYMBOL(match_token);
>  EXPORT_SYMBOL(match_int);
>  EXPORT_SYMBOL(match_octal);
>  EXPORT_SYMBOL(match_hex);
> -EXPORT_SYMBOL(match_strcpy);
> +EXPORT_SYMBOL(match_strlcpy);
>  EXPORT_SYMBOL(match_strdup);

It would be better to present this as two patches.  One adds the new core
APIs and the other uses those APIs in v9fs.  The patches would take
separate routes into mainline.

I guess I can sneak this one in as-is, as long as the v9fs guys are OK with
that?

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

* Re: [PATCH] Make v9fs uname and remotename parsing more robust
  2008-02-23  8:07 ` [PATCH] Make v9fs " Andrew Morton
@ 2008-02-24 15:37   ` Eric Van Hensbergen
  2008-02-24 22:17     ` Andrew Morton
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Van Hensbergen @ 2008-02-24 15:37 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Markus Armbruster, linux-kernel, Latchesar Ionkov, Jim Meyering

On Sat, Feb 23, 2008 at 2:07 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>
>  It would be better to present this as two patches.  One adds the new core
>  APIs and the other uses those APIs in v9fs.  The patches would take
>  separate routes into mainline.
>
>  I guess I can sneak this one in as-is, as long as the v9fs guys are OK with
>  that?
>

I'm fine with it.  Shall I pull it through the v9fs-devel patch line
or would you rather send it with your patches Andrew?

        -eric

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

* Re: [PATCH] Make v9fs uname and remotename parsing more robust
  2008-02-24 15:37   ` Eric Van Hensbergen
@ 2008-02-24 22:17     ` Andrew Morton
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Morton @ 2008-02-24 22:17 UTC (permalink / raw)
  To: Eric Van Hensbergen
  Cc: Markus Armbruster, linux-kernel, Latchesar Ionkov, Jim Meyering

On Sun, 24 Feb 2008 09:37:23 -0600 "Eric Van Hensbergen" <ericvh@gmail.com> wrote:

> On Sat, Feb 23, 2008 at 2:07 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >
> >  It would be better to present this as two patches.  One adds the new core
> >  APIs and the other uses those APIs in v9fs.  The patches would take
> >  separate routes into mainline.
> >
> >  I guess I can sneak this one in as-is, as long as the v9fs guys are OK with
> >  that?
> >
> 
> I'm fine with it.  Shall I pull it through the v9fs-devel patch line
> or would you rather send it with your patches Andrew?
> 

It's more likely to get some testing in your tree, thanks.  I'll send over
my version with improved title and my s-o-b.

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

end of thread, other threads:[~2008-02-24 22:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-15 17:54 [PATCH] Make v9fs uname and remotename parsing more robust Markus Armbruster
2008-02-22 15:57 ` [PATCH] 9p: Make " Markus Armbruster
2008-02-23  8:07 ` [PATCH] Make v9fs " Andrew Morton
2008-02-24 15:37   ` Eric Van Hensbergen
2008-02-24 22:17     ` Andrew Morton

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).