* [PATCH] staging: lustre: Remove VLA usage
@ 2018-03-07 5:46 Kees Cook
2018-03-07 10:02 ` Tobin C. Harding
2018-03-07 13:10 ` Rasmus Villemoes
0 siblings, 2 replies; 6+ messages in thread
From: Kees Cook @ 2018-03-07 5:46 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Tobin C. Harding, Tycho Andersen, Oleg Drokin,
Andreas Dilger, James Simmons, Greg Kroah-Hartman, Dmitry Eremin,
Gargi Sharma, lustre-devel, devel, Kernel Hardening
The kernel would like to remove all VLA usage. This switches to a
simple kasprintf() instead.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
index 532384c91447..aab4eab64289 100644
--- a/drivers/staging/lustre/lustre/llite/xattr.c
+++ b/drivers/staging/lustre/lustre/llite/xattr.c
@@ -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)
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: lustre: Remove VLA usage
2018-03-07 5:46 [PATCH] staging: lustre: Remove VLA usage Kees Cook
@ 2018-03-07 10:02 ` Tobin C. Harding
2018-03-07 13:10 ` Rasmus Villemoes
1 sibling, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2018-03-07 10:02 UTC (permalink / raw)
To: Kees Cook
Cc: devel, Dmitry Eremin, Tycho Andersen, Andreas Dilger,
Kernel Hardening, Greg Kroah-Hartman, linux-kernel, Gargi Sharma,
Oleg Drokin, lustre-devel
On Tue, Mar 06, 2018 at 09:46:08PM -0800, Kees Cook wrote:
> The kernel would like to remove all VLA usage. This switches to a
> simple kasprintf() instead.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> index 532384c91447..aab4eab64289 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -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);
This is cool. We've had kasprintf() since 2007, who knew?!
thanks,
Tobin.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: lustre: Remove VLA usage
2018-03-07 5:46 [PATCH] staging: lustre: Remove VLA usage Kees Cook
2018-03-07 10:02 ` Tobin C. Harding
@ 2018-03-07 13:10 ` Rasmus Villemoes
2018-03-07 17:20 ` Kees Cook
2018-03-08 10:43 ` Dan Carpenter
1 sibling, 2 replies; 6+ messages in thread
From: Rasmus Villemoes @ 2018-03-07 13:10 UTC (permalink / raw)
To: Kees Cook, Greg Kroah-Hartman
Cc: linux-kernel, Tobin C. Harding, Tycho Andersen, Oleg Drokin,
Andreas Dilger, James Simmons, Dmitry Eremin, Gargi Sharma,
lustre-devel, devel, Kernel Hardening
On 2018-03-07 06:46, Kees Cook wrote:
> The kernel would like to remove all VLA usage. This switches to a
> simple kasprintf() instead.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> index 532384c91447..aab4eab64289 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -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);
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'.
> + 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);
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: lustre: Remove VLA usage
2018-03-07 13:10 ` Rasmus Villemoes
@ 2018-03-07 17:20 ` Kees Cook
2018-03-07 20:48 ` Rasmus Villemoes
2018-03-08 10:43 ` Dan Carpenter
1 sibling, 1 reply; 6+ messages in thread
From: Kees Cook @ 2018-03-07 17:20 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: devel, Dmitry Eremin, Tycho Andersen, Andreas Dilger,
Kernel Hardening, Greg Kroah-Hartman, LKML, Gargi Sharma,
Oleg Drokin, Lustre Development List
On Wed, Mar 7, 2018 at 5:10 AM, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
> On 2018-03-07 06:46, Kees Cook wrote:
>> The kernel would like to remove all VLA usage. This switches to a
>> simple kasprintf() instead.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
>> index 532384c91447..aab4eab64289 100644
>> --- a/drivers/staging/lustre/lustre/llite/xattr.c
>> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
>> @@ -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);
>
> 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'.
>
>> + 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);
>
> 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
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
--
Kees Cook
Pixel Security
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: lustre: Remove VLA usage
2018-03-07 17:20 ` Kees Cook
@ 2018-03-07 20:48 ` Rasmus Villemoes
0 siblings, 0 replies; 6+ messages in thread
From: Rasmus Villemoes @ 2018-03-07 20:48 UTC (permalink / raw)
To: Kees Cook
Cc: devel, Dmitry Eremin, Tycho Andersen, Andreas Dilger,
Rasmus Villemoes, Greg Kroah-Hartman, Kernel Hardening, LKML,
Gargi Sharma, Oleg Drokin, Lustre Development List
On Wed, Mar 07 2018, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Mar 7, 2018 at 5:10 AM, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>> On 2018-03-07 06:46, Kees Cook wrote:
>>> 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'.
>>
>
> 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 <linux@rasmusvillemoes.dk>
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.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: lustre: Remove VLA usage
2018-03-07 13:10 ` Rasmus Villemoes
2018-03-07 17:20 ` Kees Cook
@ 2018-03-08 10:43 ` Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2018-03-08 10:43 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: devel, Dmitry Eremin, Tycho Andersen, Andreas Dilger, Kees Cook,
Kernel Hardening, Greg Kroah-Hartman, linux-kernel, Gargi Sharma,
Oleg Drokin, lustre-devel
On Wed, Mar 07, 2018 at 02:10:41PM +0100, Rasmus Villemoes wrote:
> On 2018-03-07 06:46, Kees Cook wrote:
> > The kernel would like to remove all VLA usage. This switches to a
> > simple kasprintf() instead.
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > drivers/staging/lustre/lustre/llite/xattr.c | 19 +++++++++++++------
> > 1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> > index 532384c91447..aab4eab64289 100644
> > --- a/drivers/staging/lustre/lustre/llite/xattr.c
> > +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> > @@ -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);
>
> 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'.
>
> > + 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);
>
> 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
That kind of symbolic manipulation is crazy hard to do.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-08 10:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-07 5:46 [PATCH] staging: lustre: Remove VLA usage Kees Cook
2018-03-07 10:02 ` Tobin C. Harding
2018-03-07 13:10 ` Rasmus Villemoes
2018-03-07 17:20 ` Kees Cook
2018-03-07 20:48 ` Rasmus Villemoes
2018-03-08 10:43 ` Dan Carpenter
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).