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 ma...
[ message continues ]

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?

--

On Sat, Feb 23, 2008 at 2:07 AM, Andrew Morton
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

--

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.

--

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 m...
[ message continues ]