[PATCH libdrm] xf86drm: Bound strstr() to the allocated data

[PATCH libdrm] xf86drm: Bound strstr() to the allocated data

[Damien Lespiau]

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';
     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

Re: [Intel-gfx] [PATCH libdrm] xf86drm: Bound strstr() to the allocated data

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Bound strstr() to the allocated data

[Damien Lespiau]

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

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx