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

Submitted by Damien Lespiau on Jan. 22, 2016, 12:51 p.m.

Details

Message ID 1453467083-24797-1-git-send-email-damien.lespiau@intel.com
State New
Headers show
Series "xf86drm: Bound strstr() to the allocated data" ( rev: 1 ) in libdrm - Intel

Not browsing as part of any series.

Commit Message

Damien Lespiau Jan. 22, 2016, 12:51 p.m.
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(-)

Patch hide | download patch | download mbox

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;

Comments

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
>