* [Qemu-devel] [PATCH] file-posix: clean up max_segments buffer termination
@ 2017-03-14 9:09 Stefan Hajnoczi
2017-03-14 9:25 ` Kevin Wolf
2017-03-14 9:42 ` Fam Zheng
0 siblings, 2 replies; 3+ messages in thread
From: Stefan Hajnoczi @ 2017-03-14 9:09 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, qemu-block, Fam Zheng, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] file-posix: clean up max_segments buffer termination
2017-03-14 9:09 [Qemu-devel] [PATCH] file-posix: clean up max_segments buffer termination Stefan Hajnoczi
@ 2017-03-14 9:25 ` Kevin Wolf
2017-03-14 9:42 ` Fam Zheng
1 sibling, 0 replies; 3+ messages in thread
From: Kevin Wolf @ 2017-03-14 9:25 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, qemu-block, Fam Zheng
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] file-posix: clean up max_segments buffer termination
2017-03-14 9:09 [Qemu-devel] [PATCH] file-posix: clean up max_segments buffer termination Stefan Hajnoczi
2017-03-14 9:25 ` Kevin Wolf
@ 2017-03-14 9:42 ` Fam Zheng
1 sibling, 0 replies; 3+ messages in thread
From: Fam Zheng @ 2017-03-14 9:42 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, Kevin Wolf, qemu-block
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-14 9:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-14 9:09 [Qemu-devel] [PATCH] file-posix: clean up max_segments buffer termination Stefan Hajnoczi
2017-03-14 9:25 ` Kevin Wolf
2017-03-14 9:42 ` Fam Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.