[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

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

[Dave Gordon]

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
>

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