| Message ID | 53121A4B.70308@gmail.com |
|---|---|
| State | New |
| Headers | show |
Chen Gang <gang.chen.5i5j@gmail.com> writes: > 'ctx->fs_root' + 'path'/'fullname.data' may be larger than PATH_MAX, so > need use snprintf() instead of sprintf() just like another area have done in 9pfs. > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > hw/9pfs/virtio-9p-local.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c > index 77a04cd..61be75a 100644 > --- a/hw/9pfs/virtio-9p-local.c > +++ b/hw/9pfs/virtio-9p-local.c > @@ -898,7 +898,8 @@ static int local_remove(FsContext *ctx, const char *path) > * directory > */ > if (S_ISDIR(stbuf.st_mode)) { > - sprintf(buffer, "%s/%s/%s", ctx->fs_root, path, VIRTFS_META_DIR); > + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", > + ctx->fs_root, path, VIRTFS_META_DIR); > err = remove(buffer); > if (err < 0 && errno != ENOENT) { > /* > @@ -1033,8 +1034,8 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir, > * If directory remove .virtfs_metadata contained in the > * directory > */ > - sprintf(buffer, "%s/%s/%s", ctx->fs_root, > - fullname.data, VIRTFS_META_DIR); > + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", ctx->fs_root, > + fullname.data, VIRTFS_META_DIR); > ret = remove(buffer); > if (ret < 0 && errno != ENOENT) { > /* Turns a buffer overrun bug into a truncation bug. The next commit fixes truncation bugs including this one. Would be nice to spell this out in the commit message. Perhaps Aneesh can do it on commit.
On 03/03/2014 04:34 PM, Markus Armbruster wrote: > Chen Gang <gang.chen.5i5j@gmail.com> writes: > >> 'ctx->fs_root' + 'path'/'fullname.data' may be larger than PATH_MAX, so >> need use snprintf() instead of sprintf() just like another area have done in 9pfs. >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> --- >> hw/9pfs/virtio-9p-local.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c >> index 77a04cd..61be75a 100644 >> --- a/hw/9pfs/virtio-9p-local.c >> +++ b/hw/9pfs/virtio-9p-local.c >> @@ -898,7 +898,8 @@ static int local_remove(FsContext *ctx, const char *path) >> * directory >> */ >> if (S_ISDIR(stbuf.st_mode)) { >> - sprintf(buffer, "%s/%s/%s", ctx->fs_root, path, VIRTFS_META_DIR); >> + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", >> + ctx->fs_root, path, VIRTFS_META_DIR); >> err = remove(buffer); >> if (err < 0 && errno != ENOENT) { >> /* >> @@ -1033,8 +1034,8 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir, >> * If directory remove .virtfs_metadata contained in the >> * directory >> */ >> - sprintf(buffer, "%s/%s/%s", ctx->fs_root, >> - fullname.data, VIRTFS_META_DIR); >> + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", ctx->fs_root, >> + fullname.data, VIRTFS_META_DIR); >> ret = remove(buffer); >> if (ret < 0 && errno != ENOENT) { >> /* > > Turns a buffer overrun bug into a truncation bug. The next commit fixes > truncation bugs including this one. Would be nice to spell this out in > the commit message. Perhaps Aneesh can do it on commit. > Please help doing it on commit. Thanks.
Chen Gang <gang.chen.5i5j@gmail.com> writes: > On 03/03/2014 04:34 PM, Markus Armbruster wrote: >> Chen Gang <gang.chen.5i5j@gmail.com> writes: >> >>> 'ctx->fs_root' + 'path'/'fullname.data' may be larger than PATH_MAX, so >>> need use snprintf() instead of sprintf() just like another area have done in 9pfs. >>> >>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >>> --- >>> hw/9pfs/virtio-9p-local.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c >>> index 77a04cd..61be75a 100644 >>> --- a/hw/9pfs/virtio-9p-local.c >>> +++ b/hw/9pfs/virtio-9p-local.c >>> @@ -898,7 +898,8 @@ static int local_remove(FsContext *ctx, const char *path) >>> * directory >>> */ >>> if (S_ISDIR(stbuf.st_mode)) { >>> - sprintf(buffer, "%s/%s/%s", ctx->fs_root, path, VIRTFS_META_DIR); >>> + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", >>> + ctx->fs_root, path, VIRTFS_META_DIR); >>> err = remove(buffer); >>> if (err < 0 && errno != ENOENT) { >>> /* >>> @@ -1033,8 +1034,8 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir, >>> * If directory remove .virtfs_metadata contained in the >>> * directory >>> */ >>> - sprintf(buffer, "%s/%s/%s", ctx->fs_root, >>> - fullname.data, VIRTFS_META_DIR); >>> + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", ctx->fs_root, >>> + fullname.data, VIRTFS_META_DIR); >>> ret = remove(buffer); >>> if (ret < 0 && errno != ENOENT) { >>> /* >> >> Turns a buffer overrun bug into a truncation bug. The next commit fixes >> truncation bugs including this one. Would be nice to spell this out in >> the commit message. Perhaps Aneesh can do it on commit. >> > > Please help doing it on commit. If you respin your series anyway, simply improve your commit message. Something like this would do: Gcc: nnml:mail.redhat.xlst.qemu-devel From: Markus Armbruster <armbru@redhat.com> --text follows this line-- Chen Gang <gang.chen.5i5j@gmail.com> writes: > On 03/03/2014 04:34 PM, Markus Armbruster wrote: >> Chen Gang <gang.chen.5i5j@gmail.com> writes: >> >>> 'ctx->fs_root' + 'path'/'fullname.data' may be larger than PATH_MAX, so >>> need use snprintf() instead of sprintf() just like another area have done in 9pfs. >>> >>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >>> --- >>> hw/9pfs/virtio-9p-local.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c >>> index 77a04cd..61be75a 100644 >>> --- a/hw/9pfs/virtio-9p-local.c >>> +++ b/hw/9pfs/virtio-9p-local.c >>> @@ -898,7 +898,8 @@ static int local_remove(FsContext *ctx, const char *path) >>> * directory >>> */ >>> if (S_ISDIR(stbuf.st_mode)) { >>> - sprintf(buffer, "%s/%s/%s", ctx->fs_root, path, VIRTFS_META_DIR); >>> + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", >>> + ctx->fs_root, path, VIRTFS_META_DIR); >>> err = remove(buffer); >>> if (err < 0 && errno != ENOENT) { >>> /* >>> @@ -1033,8 +1034,8 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir, >>> * If directory remove .virtfs_metadata contained in the >>> * directory >>> */ >>> - sprintf(buffer, "%s/%s/%s", ctx->fs_root, >>> - fullname.data, VIRTFS_META_DIR); >>> + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", ctx->fs_root, >>> + fullname.data, VIRTFS_META_DIR); >>> ret = remove(buffer); >>> if (ret < 0 && errno != ENOENT) { >>> /* >> >> Turns a buffer overrun bug into a truncation bug. The next commit fixes >> truncation bugs including this one. Would be nice to spell this out in >> the commit message. Perhaps Aneesh can do it on commit. >> > > Please help doing it on commit. If you need to respin your series anyway, simply improve your commit message. Something like this would do: hw/9pfs: Fix buffer overrun in local_remove(), local_unlinkat() When 'ctx->fs_root' + 'path'/'fullname.data' is larger than PATH_MAX, we overrunning a buffer, smashing the stack. Fix by switching from sprintf() to snprintf(). Turns the buffer overrun bugs into truncation bugs. The next commit will fix them along with similar truncation bugs elsewhere in 9pfs.
Chen Gang <gang.chen.5i5j@gmail.com> writes: > On 03/03/2014 04:34 PM, Markus Armbruster wrote: >> Chen Gang <gang.chen.5i5j@gmail.com> writes: >> >>> 'ctx->fs_root' + 'path'/'fullname.data' may be larger than PATH_MAX, so >>> need use snprintf() instead of sprintf() just like another area have done in 9pfs. >>> >>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >>> --- >>> hw/9pfs/virtio-9p-local.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c >>> index 77a04cd..61be75a 100644 >>> --- a/hw/9pfs/virtio-9p-local.c >>> +++ b/hw/9pfs/virtio-9p-local.c >>> @@ -898,7 +898,8 @@ static int local_remove(FsContext *ctx, const char *path) >>> * directory >>> */ >>> if (S_ISDIR(stbuf.st_mode)) { >>> - sprintf(buffer, "%s/%s/%s", ctx->fs_root, path, VIRTFS_META_DIR); >>> + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", >>> + ctx->fs_root, path, VIRTFS_META_DIR); >>> err = remove(buffer); >>> if (err < 0 && errno != ENOENT) { >>> /* >>> @@ -1033,8 +1034,8 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir, >>> * If directory remove .virtfs_metadata contained in the >>> * directory >>> */ >>> - sprintf(buffer, "%s/%s/%s", ctx->fs_root, >>> - fullname.data, VIRTFS_META_DIR); >>> + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", ctx->fs_root, >>> + fullname.data, VIRTFS_META_DIR); >>> ret = remove(buffer); >>> if (ret < 0 && errno != ENOENT) { >>> /* >> >> Turns a buffer overrun bug into a truncation bug. The next commit fixes >> truncation bugs including this one. Would be nice to spell this out in >> the commit message. Perhaps Aneesh can do it on commit. >> > > Please help doing it on commit. Will update when i am applyting this to my tree. -aneesh
Chen Gang <gang.chen.5i5j@gmail.com> writes: > 'ctx->fs_root' + 'path'/'fullname.data' may be larger than PATH_MAX, so > need use snprintf() instead of sprintf() just like another area have done in 9pfs. > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> Will take care of Markus feedback. Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> > --- > hw/9pfs/virtio-9p-local.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c > index 77a04cd..61be75a 100644 > --- a/hw/9pfs/virtio-9p-local.c > +++ b/hw/9pfs/virtio-9p-local.c > @@ -898,7 +898,8 @@ static int local_remove(FsContext *ctx, const char *path) > * directory > */ > if (S_ISDIR(stbuf.st_mode)) { > - sprintf(buffer, "%s/%s/%s", ctx->fs_root, path, VIRTFS_META_DIR); > + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", > + ctx->fs_root, path, VIRTFS_META_DIR); > err = remove(buffer); > if (err < 0 && errno != ENOENT) { > /* > @@ -1033,8 +1034,8 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir, > * If directory remove .virtfs_metadata contained in the > * directory > */ > - sprintf(buffer, "%s/%s/%s", ctx->fs_root, > - fullname.data, VIRTFS_META_DIR); > + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", ctx->fs_root, > + fullname.data, VIRTFS_META_DIR); > ret = remove(buffer); > if (ret < 0 && errno != ENOENT) { > /* > -- > 1.7.11.7
On 03/03/2014 10:42 PM, Markus Armbruster wrote: > Chen Gang <gang.chen.5i5j@gmail.com> writes: > >> On 03/03/2014 04:34 PM, Markus Armbruster wrote: >>> Turns a buffer overrun bug into a truncation bug. The next commit fixes >>> truncation bugs including this one. Would be nice to spell this out in >>> the commit message. Perhaps Aneesh can do it on commit. >>> [...] >> >> Please help doing it on commit. > [...] > If you respin your series anyway, simply improve your commit message. > Something like this would do: > > hw/9pfs: Fix buffer overrun in local_remove(), local_unlinkat() > > When 'ctx->fs_root' + 'path'/'fullname.data' is larger than > PATH_MAX, we overrunning a buffer, smashing the stack. > > Fix by switching from sprintf() to snprintf(). Turns the buffer > overrun bugs into truncation bugs. The next commit will fix them > along with similar truncation bugs elsewhere in 9pfs. > OK, thank you for your details information. And I guess, at present, I need not send patch v2 for this series (Aneesh has helped done for them). Thanks.
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c index 77a04cd..61be75a 100644 --- a/hw/9pfs/virtio-9p-local.c +++ b/hw/9pfs/virtio-9p-local.c @@ -898,7 +898,8 @@ static int local_remove(FsContext *ctx, const char *path) * directory */ if (S_ISDIR(stbuf.st_mode)) { - sprintf(buffer, "%s/%s/%s", ctx->fs_root, path, VIRTFS_META_DIR); + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", + ctx->fs_root, path, VIRTFS_META_DIR); err = remove(buffer); if (err < 0 && errno != ENOENT) { /* @@ -1033,8 +1034,8 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir, * If directory remove .virtfs_metadata contained in the * directory */ - sprintf(buffer, "%s/%s/%s", ctx->fs_root, - fullname.data, VIRTFS_META_DIR); + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", ctx->fs_root, + fullname.data, VIRTFS_META_DIR); ret = remove(buffer); if (ret < 0 && errno != ENOENT) { /*
'ctx->fs_root' + 'path'/'fullname.data' may be larger than PATH_MAX, so need use snprintf() instead of sprintf() just like another area have done in 9pfs. Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- hw/9pfs/virtio-9p-local.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)