Skip to contentSkip to conversation

staging: lustre: Remove VLA usage

PATCH Superseded Kees Cook 5 participants 6 messages Mar 7, 2018 – Mar 8, 2018 Staging

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 staging: lustre: Remove VLA usage

The kernel would like to remove all VLA usage. This switches to a simple kasprintf() instead.

Signed-off-by Kees Cook <[email protected]>
---
 drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/staging/lustre/lustre/llite/xattr.c +13 -6
index 532384c91447..aab4eab64289 100644
@@ -87,7 +87,7 @@ ll_xattr_set_common(const struct xattr_handler *handler,
 		    const char *name, const void *value, size_t size,
 		    int flags)
 {
-	char fullname[strlen(handler->prefix) + strlen(name) + 1];
+	char *fullname;
 	struct ll_sb_info *sbi = ll_i2sbi(inode);
 	struct ptlrpc_request *req = NULL;
 	const char *pv = value;

@@ -141,10 +141,13 @@ ll_xattr_set_common(const struct xattr_handler *handler,
 			return -EPERM;
 	}
 
-	sprintf(fullname, "%s%s\n", handler->prefix, name);
+	fullname = kasprintf(GFP_KERNEL, "%s%s\n", handler->prefix, name);
+	if (!fullname)
+		return -ENOMEM;
 	rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
 			 valid, fullname, pv, size, 0, flags,
 			 ll_i2suppgid(inode), &req);
+	kfree(fullname);
 	if (rc) {
 		if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
 			LCONSOLE_INFO("Disabling user_xattr feature because it is not supported on the server\n");

@@ -364,7 +367,7 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
 			       struct dentry *dentry, struct inode *inode,
 			       const char *name, void *buffer, size_t size)
 {
-	char fullname[strlen(handler->prefix) + strlen(name) + 1];
+	char *fullname;
 	struct ll_sb_info *sbi = ll_i2sbi(inode);
 #ifdef CONFIG_FS_POSIX_ACL
 	struct ll_inode_info *lli = ll_i2info(inode);

@@ -411,9 +414,13 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
 	if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
 		return -ENODATA;
 #endif
-	sprintf(fullname, "%s%s\n", handler->prefix, name);
-	return ll_xattr_list(inode, fullname, handler->flags, buffer, size,
-			     OBD_MD_FLXATTR);
+	fullname = kasprintf(GFP_KERNEL, "%s%s\n", handler->prefix, name);
+	if (!fullname)
+		return -ENOMEM;
+	rc = ll_xattr_list(inode, fullname, handler->flags, buffer, size,
+			   OBD_MD_FLXATTR);
+	kfree(fullname);
+	return rc;
 }
 
 static ssize_t ll_getxattr_lov(struct inode *inode, void *buf, size_t buf_size)
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security
#1 lore raw
Quoted 23 lines · Kees Cook

The kernel would like to remove all VLA usage. This switches to a simple kasprintf() instead.

Signed-off-by Kees Cook <[email protected]>
---
 drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/staging/lustre/lustre/llite/xattr.c +1 -1
index 532384c91447..aab4eab64289 100644
@@ -87,7 +87,7 @@ ll_xattr_set_common(const struct xattr_handler *handler,
 		    const char *name, const void *value, size_t size,
 		    int flags)
 {
-	char fullname[strlen(handler->prefix) + strlen(name) + 1];
+	char *fullname;
 	struct ll_sb_info *sbi = ll_i2sbi(inode);
 	struct ptlrpc_request *req = NULL;
 	const char *pv = value;

@@ -141,10 +141,13 @@ ll_xattr_set_common(const struct xattr_handler *handler,
 			return -EPERM;

drivers/staging/lustre/lustre/llite/xattr.c ll_xattr_set_common(const struct xattr_handler *handler, · line 150

Quoted 10 lines
drivers/staging/lustre/lustre/llite/xattr.c +4 -1
 	}
 
-	sprintf(fullname, "%s%s\n", handler->prefix, name);
+	fullname = kasprintf(GFP_KERNEL, "%s%s\n", handler->prefix, name);
+	if (!fullname)
+		return -ENOMEM;
 	rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
 			 valid, fullname, pv, size, 0, flags,
 			 ll_i2suppgid(inode), &req);
+	kfree(fullname);

This is cool. We've had kasprintf() since 2007, who knew?!

thanks,
Tobin.

#2 lore raw
In reply to Kees Cook
Quoted 22 lines · Kees Cook

The kernel would like to remove all VLA usage. This switches to a simple kasprintf() instead.

Signed-off-by Kees Cook <[email protected]>
---
 drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/staging/lustre/lustre/llite/xattr.c +1 -1
index 532384c91447..aab4eab64289 100644
@@ -87,7 +87,7 @@ ll_xattr_set_common(const struct xattr_handler *handler,
 		    const char *name, const void *value, size_t size,
 		    int flags)
 {
-	char fullname[strlen(handler->prefix) + strlen(name) + 1];
+	char *fullname;
 	struct ll_sb_info *sbi = ll_i2sbi(inode);
 	struct ptlrpc_request *req = NULL;
 	const char *pv = value;

drivers/staging/lustre/lustre/llite/xattr.c ll_xattr_set_common(const struct xattr_handler *handler, · line 144

Quoted 4 lines
drivers/staging/lustre/lustre/llite/xattr.c +0 -1
@@ -141,10 +141,13 @@ ll_xattr_set_common(const struct xattr_handler *handler,
 			return -EPERM;
 	}
 
-	sprintf(fullname, "%s%s\n", handler->prefix, name);

It's probably worth pointing out that this actually fixes an unconditional buffer overflow: fullname only has room for the two strings and the '\n', but vsnprintf() is told that the buffer has infinite size (well, INT_MAX), so there should be plenty of room to append the '\0' after the '\n'.

Quoted 20 lines
drivers/staging/lustre/lustre/llite/xattr.c +5 -1
+	fullname = kasprintf(GFP_KERNEL, "%s%s\n", handler->prefix, name);
+	if (!fullname)
+		return -ENOMEM;
 	rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
 			 valid, fullname, pv, size, 0, flags,
 			 ll_i2suppgid(inode), &req);
+	kfree(fullname);
 	if (rc) {
 		if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
 			LCONSOLE_INFO("Disabling user_xattr feature because it is not supported on the server\n");

@@ -364,7 +367,7 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
 			       struct dentry *dentry, struct inode *inode,
 			       const char *name, void *buffer, size_t size)
 {
-	char fullname[strlen(handler->prefix) + strlen(name) + 1];
+	char *fullname;
 	struct ll_sb_info *sbi = ll_i2sbi(inode);
 #ifdef CONFIG_FS_POSIX_ACL
 	struct ll_inode_info *lli = ll_i2info(inode);

drivers/staging/lustre/lustre/llite/xattr.c static int ll_xattr_get_common(const struct xattr_handler *handler, · line 414

Quoted 4 lines
drivers/staging/lustre/lustre/llite/xattr.c +0 -1
@@ -411,9 +414,13 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
 	if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
 		return -ENODATA;
 #endif
-	sprintf(fullname, "%s%s\n", handler->prefix, name);

Same here.

I'm a little surprised this hasn't been caugt by static analysis, I thought gcc/coverity/smatch/whatnot had gotten pretty good at computing the size of the output generated by a given format string with "known" arguments and comparing to the size of the output buffer. Though of course it does require the tool to be able to do symbolic manipulations, in this case realizing that

outsize == strlen(x)+strlen(y)+1+1 > bufsize == strlen(x)+strlen(y)+1

Rasmus

author
#3 lore raw
Quoted 68 lines · Rasmus Villemoes
Quoted 22 lines · Kees Cook

The kernel would like to remove all VLA usage. This switches to a simple kasprintf() instead.

Signed-off-by Kees Cook <[email protected]>
---
 drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/staging/lustre/lustre/llite/xattr.c +1 -1
index 532384c91447..aab4eab64289 100644
@@ -87,7 +87,7 @@ ll_xattr_set_common(const struct xattr_handler *handler,
                  const char *name, const void *value, size_t size,
                  int flags)
 {
-     char fullname[strlen(handler->prefix) + strlen(name) + 1];
+     char *fullname;
      struct ll_sb_info *sbi = ll_i2sbi(inode);
      struct ptlrpc_request *req = NULL;
      const char *pv = value;

drivers/staging/lustre/lustre/llite/xattr.c ll_xattr_set_common(const struct xattr_handler *handler, · line 144

Quoted 4 lines
drivers/staging/lustre/lustre/llite/xattr.c +0 -1
@@ -141,10 +141,13 @@ ll_xattr_set_common(const struct xattr_handler *handler,
                      return -EPERM;
      }
 
-     sprintf(fullname, "%s%s\n", handler->prefix, name);

It's probably worth pointing out that this actually fixes an unconditional buffer overflow: fullname only has room for the two strings and the '\n', but vsnprintf() is told that the buffer has infinite size (well, INT_MAX), so there should be plenty of room to append the '\0' after the '\n'.

Quoted 20 lines
drivers/staging/lustre/lustre/llite/xattr.c +5 -1
+     fullname = kasprintf(GFP_KERNEL, "%s%s\n", handler->prefix, name);
+     if (!fullname)
+             return -ENOMEM;
      rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
                       valid, fullname, pv, size, 0, flags,
                       ll_i2suppgid(inode), &req);
+     kfree(fullname);
      if (rc) {
              if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
                      LCONSOLE_INFO("Disabling user_xattr feature because it is not supported on the server\n");

@@ -364,7 +367,7 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
                             struct dentry *dentry, struct inode *inode,
                             const char *name, void *buffer, size_t size)
 {
-     char fullname[strlen(handler->prefix) + strlen(name) + 1];
+     char *fullname;
      struct ll_sb_info *sbi = ll_i2sbi(inode);
 #ifdef CONFIG_FS_POSIX_ACL
      struct ll_inode_info *lli = ll_i2info(inode);

drivers/staging/lustre/lustre/llite/xattr.c static int ll_xattr_get_common(const struct xattr_handler *handler, · line 414

Quoted 4 lines
drivers/staging/lustre/lustre/llite/xattr.c +0 -1
@@ -411,9 +414,13 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
      if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
              return -ENODATA;
 #endif
-     sprintf(fullname, "%s%s\n", handler->prefix, name);

Same here.

I'm a little surprised this hasn't been caugt by static analysis, I thought gcc/coverity/smatch/whatnot had gotten pretty good at computing the size of the output generated by a given format string with "known" arguments and comparing to the size of the output buffer. Though of course it does require the tool to be able to do symbolic manipulations, in this case realizing that

Quoted 3 lines

outsize == strlen(x)+strlen(y)+1+1 > bufsize == strlen(x)+strlen(y)+1

Rasmus

Oh yes, hah. I didn't even see the \n in the string. :P

So, both a VLA fix and a buffer over-run fix. Can I add your "Reviewed-by"? :)

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
Reviewed-by
#4 lore raw
Quoted 14 lines · Kees Cook
Quoted 11 lines · Rasmus Villemoes
Quoted 3 lines · Kees Cook

The kernel would like to remove all VLA usage. This switches to a simple kasprintf() instead.

It's probably worth pointing out that this actually fixes an unconditional buffer overflow: fullname only has room for the two strings and the '\n', but vsnprintf() is told that the buffer has infinite size (well, INT_MAX), so there should be plenty of room to append the '\0' after the '\n'.

Quoted 3 lines

Oh yes, hah. I didn't even see the \n in the string. :P

So, both a VLA fix and a buffer over-run fix. Can I add your "Reviewed-by"? :)

Sure,

Reviewed-by Rasmus Villemoes <[email protected]>

A nit, if you're resending anyway: can you move the "char *fullname" declarations down a bit, to between pv,valid, and lli,rc, respectively? That keeps the initialized and uninitialized variables nicely together and ends up looking better.

#5 lore raw
In reply to Rasmus Villemoes
Quoted 61 lines · Rasmus Villemoes
Quoted 22 lines · Kees Cook

The kernel would like to remove all VLA usage. This switches to a simple kasprintf() instead.

Signed-off-by Kees Cook <[email protected]>
---
 drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/staging/lustre/lustre/llite/xattr.c +1 -1
index 532384c91447..aab4eab64289 100644
@@ -87,7 +87,7 @@ ll_xattr_set_common(const struct xattr_handler *handler,
 		    const char *name, const void *value, size_t size,
 		    int flags)
 {
-	char fullname[strlen(handler->prefix) + strlen(name) + 1];
+	char *fullname;
 	struct ll_sb_info *sbi = ll_i2sbi(inode);
 	struct ptlrpc_request *req = NULL;
 	const char *pv = value;

drivers/staging/lustre/lustre/llite/xattr.c ll_xattr_set_common(const struct xattr_handler *handler, · line 144

Quoted 4 lines
drivers/staging/lustre/lustre/llite/xattr.c +0 -1
@@ -141,10 +141,13 @@ ll_xattr_set_common(const struct xattr_handler *handler,
 			return -EPERM;
 	}
 
-	sprintf(fullname, "%s%s\n", handler->prefix, name);

It's probably worth pointing out that this actually fixes an unconditional buffer overflow: fullname only has room for the two strings and the '\n', but vsnprintf() is told that the buffer has infinite size (well, INT_MAX), so there should be plenty of room to append the '\0' after the '\n'.

Quoted 20 lines
drivers/staging/lustre/lustre/llite/xattr.c +5 -1
+	fullname = kasprintf(GFP_KERNEL, "%s%s\n", handler->prefix, name);
+	if (!fullname)
+		return -ENOMEM;
 	rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
 			 valid, fullname, pv, size, 0, flags,
 			 ll_i2suppgid(inode), &req);
+	kfree(fullname);
 	if (rc) {
 		if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
 			LCONSOLE_INFO("Disabling user_xattr feature because it is not supported on the server\n");

@@ -364,7 +367,7 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
 			       struct dentry *dentry, struct inode *inode,
 			       const char *name, void *buffer, size_t size)
 {
-	char fullname[strlen(handler->prefix) + strlen(name) + 1];
+	char *fullname;
 	struct ll_sb_info *sbi = ll_i2sbi(inode);
 #ifdef CONFIG_FS_POSIX_ACL
 	struct ll_inode_info *lli = ll_i2info(inode);

drivers/staging/lustre/lustre/llite/xattr.c static int ll_xattr_get_common(const struct xattr_handler *handler, · line 414

Quoted 4 lines
drivers/staging/lustre/lustre/llite/xattr.c +0 -1
@@ -411,9 +414,13 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
 	if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
 		return -ENODATA;
 #endif
-	sprintf(fullname, "%s%s\n", handler->prefix, name);

Same here.

Quoted 8 lines

I'm a little surprised this hasn't been caugt by static analysis, I thought gcc/coverity/smatch/whatnot had gotten pretty good at computing the size of the output generated by a given format string with "known" arguments and comparing to the size of the output buffer. Though of course it does require the tool to be able to do symbolic manipulations, in this case realizing that

outsize == strlen(x)+strlen(y)+1+1 > bufsize == strlen(x)+strlen(y)+1

That kind of symbolic manipulation is crazy hard to do.

regards,
dan carpenter