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

[Dave Gordon <david.s.gordon@intel.com>]

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