[Qemu-devel] [PATCH] file-posix: clean up max_segments buffer termination

[Qemu-devel] [PATCH] file-posix: clean up max_segments buffer termination

[Stefan Hajnoczi]

The following pattern is unsafe:

  char buf[32];
  ret = read(fd, buf, sizeof(buf));
  ...
  buf[ret] = 0;

If read(2) returns 32 then a byte beyond the end of the buffer is
zeroed.

In practice this buffer overflow does not occur because the sysfs
max_segments file only contains an unsigned short + '\n'.  The string is
always shorter than 32 bytes.

Regardless, avoid this pattern because static analysis tools might
complain and it could lead to real buffer overflows if copy-pasted
elsewhere in the codebase.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index c4c0663..ac6bd9f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -686,7 +686,7 @@ static int hdev_get_max_segments(const struct stat *st)
         goto out;
     }
     do {
-        ret = read(fd, buf, sizeof(buf));
+        ret = read(fd, buf, sizeof(buf) - 1);
     } while (ret == -1 && errno == EINTR);
     if (ret < 0) {
         ret = -errno;
-- 
2.9.3

Re: [Qemu-devel] [PATCH] file-posix: clean up max_segments buffer termination

[Kevin Wolf]

Am 14.03.2017 um 10:09 hat Stefan Hajnoczi geschrieben:
> The following pattern is unsafe:
> 
>   char buf[32];
>   ret = read(fd, buf, sizeof(buf));
>   ...
>   buf[ret] = 0;
> 
> If read(2) returns 32 then a byte beyond the end of the buffer is
> zeroed.
> 
> In practice this buffer overflow does not occur because the sysfs
> max_segments file only contains an unsigned short + '\n'.  The string is
> always shorter than 32 bytes.
> 
> Regardless, avoid this pattern because static analysis tools might
> complain and it could lead to real buffer overflows if copy-pasted
> elsewhere in the codebase.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Oops. I should have found this during review. Thanks for catching it.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] file-posix: clean up max_segments buffer termination

[Fam Zheng]

On Tue, 03/14 17:09, Stefan Hajnoczi wrote:
> The following pattern is unsafe:
> 
>   char buf[32];
>   ret = read(fd, buf, sizeof(buf));
>   ...
>   buf[ret] = 0;
> 
> If read(2) returns 32 then a byte beyond the end of the buffer is
> zeroed.
> 
> In practice this buffer overflow does not occur because the sysfs
> max_segments file only contains an unsigned short + '\n'.  The string is
> always shorter than 32 bytes.
> 
> Regardless, avoid this pattern because static analysis tools might
> complain and it could lead to real buffer overflows if copy-pasted
> elsewhere in the codebase.

Yes that's a good point, thanks!

Fam

> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/file-posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index c4c0663..ac6bd9f 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -686,7 +686,7 @@ static int hdev_get_max_segments(const struct stat *st)
>          goto out;
>      }
>      do {
> -        ret = read(fd, buf, sizeof(buf));
> +        ret = read(fd, buf, sizeof(buf) - 1);
>      } while (ret == -1 && errno == EINTR);
>      if (ret < 0) {
>          ret = -errno;
> -- 
> 2.9.3
>