[Intel-gfx] [PATCH libdrm] xf86drm: Bound strstr() to the allocated data
On Fri, Jan 22, 2016 at 12:51:23PM +, 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
> ---
> 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ä
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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
[Intel-gfx] [PATCH libdrm] xf86drm: Bound strstr() to the allocated data
On Fri, Jan 22, 2016 at 04:48:05PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 22, 2016 at 12:51:23PM +, 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
> > ---
> > 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ä
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[].
--
Damien
[PATCH libdrm] xf86drm: Bound strstr() to the allocated data
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
---
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';
close(fd);
if (ret < 0)
return -errno;
--
2.4.3
