| Message ID | 1453467083-24797-1-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 22, 2016 at 12:51:23PM +0000, Damien Lespiau wrote: > We are reading at most sizeof(data) bytes, but then data may not contain > a terminating '\0', at least in theory, so strstr() may overflow the > stack allocated array. > > Make sure that data always contains at least one '\0'. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > xf86drm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xf86drm.c b/xf86drm.c > index 7e28b4f..5f587d9 100644 > --- a/xf86drm.c > +++ b/xf86drm.c > @@ -2863,7 +2863,7 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) > { > #ifdef __linux__ > char path[PATH_MAX + 1]; > - char data[128]; > + char data[128 + 1]; > char *str; > int domain, bus, dev, func; > int fd, ret; > @@ -2874,6 +2874,7 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) > return -errno; > > ret = read(fd, data, sizeof(data)); > + data[128] = '\0'; Slightly more paranoid would be something along the lines of if (ret >= 0) data[ret] = '\0'; But this should be good enough I think so Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> The other thing I spotted while looking at the code is the fact that it doesn't check the snprint() return value. But I guess PATH_MAX is big enough that even if you somehow make maj and min INT_MIN it'll still fit. > close(fd); > if (ret < 0) > return -errno; > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 22, 2016 at 04:48:05PM +0200, Ville Syrjälä wrote: > On Fri, Jan 22, 2016 at 12:51:23PM +0000, Damien Lespiau wrote: > > We are reading at most sizeof(data) bytes, but then data may not contain > > a terminating '\0', at least in theory, so strstr() may overflow the > > stack allocated array. > > > > Make sure that data always contains at least one '\0'. > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > --- > > xf86drm.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/xf86drm.c b/xf86drm.c > > index 7e28b4f..5f587d9 100644 > > --- a/xf86drm.c > > +++ b/xf86drm.c > > @@ -2863,7 +2863,7 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) > > { > > #ifdef __linux__ > > char path[PATH_MAX + 1]; > > - char data[128]; > > + char data[128 + 1]; > > char *str; > > int domain, bus, dev, func; > > int fd, ret; > > @@ -2874,6 +2874,7 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) > > return -errno; > > > > ret = read(fd, data, sizeof(data)); > > + data[128] = '\0'; > > Slightly more paranoid would be something along the lines of > if (ret >= 0) > data[ret] = '\0'; > > But this should be good enough I think so > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks for the review, pushed! > The other thing I spotted while looking at the code is the fact that it > doesn't check the snprint() return value. But I guess PATH_MAX is big > enough that even if you somehow make maj and min INT_MIN it'll still > fit. Right, doesn't seem we can overflow path[].
On 22/01/16 14:48, Ville Syrjälä wrote: > On Fri, Jan 22, 2016 at 12:51:23PM +0000, Damien Lespiau wrote: >> We are reading at most sizeof(data) bytes, but then data may not contain >> a terminating '\0', at least in theory, so strstr() may overflow the >> stack allocated array. >> >> Make sure that data always contains at least one '\0'. >> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> >> --- >> xf86drm.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/xf86drm.c b/xf86drm.c >> index 7e28b4f..5f587d9 100644 >> --- a/xf86drm.c >> +++ b/xf86drm.c >> @@ -2863,7 +2863,7 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) >> { >> #ifdef __linux__ >> char path[PATH_MAX + 1]; >> - char data[128]; >> + char data[128 + 1]; >> char *str; >> int domain, bus, dev, func; >> int fd, ret; >> @@ -2874,6 +2874,7 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) >> return -errno; >> >> ret = read(fd, data, sizeof(data)); >> + data[128] = '\0'; > > Slightly more paranoid would be something along the lines of > if (ret >= 0) > data[ret] = '\0'; Except that this could now be out-of-bounds :( I think the read() should be changed to not fill the newly-enlarged array completely: char data[N+1] ret = read(fd, data, N); if (ret >= 0) data[ret] = '\0'; so that in the last line, ret <= N and therefore can't overflow. But writing the NUL at the constant offset ("data[N] = '\0';") was OK too, it just means that if the data is short and not NUL-terminated it will be treated as having random bytes appended, up to the guaranteed NUL. Since the input could actually have contained those random bytes, putting the NUL at the very end of the buffer doesn't make things worse. .Dave. > But this should be good enough I think so > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The other thing I spotted while looking at the code is the fact that it > doesn't check the snprint() return value. But I guess PATH_MAX is big > enough that even if you somehow make maj and min INT_MIN it'll still > fit. > >> close(fd); >> if (ret < 0) >> return -errno; >> -- >> 2.4.3 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/xf86drm.c b/xf86drm.c index 7e28b4f..5f587d9 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -2863,7 +2863,7 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) { #ifdef __linux__ char path[PATH_MAX + 1]; - char data[128]; + char data[128 + 1]; char *str; int domain, bus, dev, func; int fd, ret; @@ -2874,6 +2874,7 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) return -errno; ret = read(fd, data, sizeof(data)); + data[128] = '\0'; close(fd); if (ret < 0) return -errno;
We are reading at most sizeof(data) bytes, but then data may not contain a terminating '\0', at least in theory, so strstr() may overflow the stack allocated array. Make sure that data always contains at least one '\0'. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- xf86drm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)