| 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[].
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(-)