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
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
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 >
© 2016 - 2024 Red Hat, Inc.