Re: [Qemu-devel] [PATCH 2/3] hw/9pfs/virtio-9p-local.c: use snprintf() instead of sprintf()

[Markus Armbruster]

Chen Gang <address@hidden> writes:

> On 03/03/2014 04:34 PM, Markus Armbruster wrote:
>> Chen Gang <address@hidden> 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 <address@hidden>
>>> ---
>>>  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 <address@hidden>
--text follows this line--
Chen Gang <address@hidden> writes:

> On 03/03/2014 04:34 PM, Markus Armbruster wrote:
>> Chen Gang <address@hidden> 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 <address@hidden>
>>> ---
>>>  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.