[PATCH libdrm] drm: Avoid out of bound write in drmOpenByName()

[PATCH libdrm] drm: Avoid out of bound write in drmOpenByName()

[Damien Lespiau]

In the fallback code that looks for devices in /proc, the read() may
return with -1 in case of error (interruption from a signal for
instance). We'll then happily write '\0' to buf[-2].

As we didn't really care about the signal interruption before, I kept it
the same way, just making sure that retcode is > 0 to avoid that case.

This was found by static analysis.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 xf86drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xf86drm.c b/xf86drm.c
index d900b4b..106b8ab 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -579,7 +579,7 @@ static int drmOpenByName(const char *name)
 	if ((fd = open(proc_name, 0, 0)) >= 0) {
 	    retcode = read(fd, buf, sizeof(buf)-1);
 	    close(fd);
-	    if (retcode) {
+	    if (retcode > 0) {
 		buf[retcode-1] = '\0';
 		for (driver = pt = buf; *pt && *pt != ' '; ++pt)
 		    ;
-- 
1.8.3.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] drm: Avoid out of bound write in drmOpenByName()

[Daniel Vetter]

On Mon, Dec 01, 2014 at 02:07:03PM +0000, Damien Lespiau wrote:
> In the fallback code that looks for devices in /proc, the read() may
> return with -1 in case of error (interruption from a signal for
> instance). We'll then happily write '\0' to buf[-2].
> 
> As we didn't really care about the signal interruption before, I kept it
> the same way, just making sure that retcode is > 0 to avoid that case.
> 
> This was found by static analysis.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

procfs support is dead, absolutely no point imo to fix it. From the
kernel:

commit cb6458f97b53d7f73043206c18014b3ca63ac345
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Aug 8 15:41:34 2013 +0200

    drm: remove procfs code, take 2
    
    So almost two years ago I've tried to nuke the procfs code already
    once before:
    
    http://lists.freedesktop.org/archives/dri-devel/2011-October/015707.html
    
    The conclusion was that userspace drivers (specifically libdrm device
    node detection) stopped relying on procfs in 2001. But after some
    digging it turned out that the drmstat tool in libdrm is still using
    those files (but only when certain options are set). So we've decided
    to keep profcs.
    
    But I when I've started to dig around again what exactly this tool
    does I've noticed that it tries to read the "mem", "vm", and "vma"
    files from procfs. Now as far my git history digging shows "mem" never
    did anything useful (at least in the version that first showed up in
    upstream history in 2004) and the file was remove in
    
    commit 955b12def42e83287c1bdb1411d99451753c1391
    Author: Ben Gamari <bgamari@gmail.com>
    Date:   Tue Feb 17 20:08:49 2009 -0500
    
        drm: Convert proc files to seq_file and introduce debugfs
    
    Which means that for over 4 years drmstat has been broken, and no one
    cared. In my opinion that's proof enough that no one is actually using
    drmstat, and so that we can savely nuke the procfs support from drm.
    
    While at it fix up the error case cleanup for debugfs in drm_get_minor.
    
    v2: Fix dates, libdrm stopped relying on procfs for drm node detection
    in 2001.
    
    v3: fixup compilation warning for !CONFIG_DEBUG_FS, reported by
    Fengguang Wu.
    
    Cc: kbuild test robot <fengguang.wu@intel.com>
    Cc: Dave Airlie <airlied@linux.ie>
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
    Signed-off-by: Dave Airlie <airlied@redhat.com>


Instead I think we should just rip this fallback code out. Volunteered?
You can reuse the digging I've done for the kernel side.

Cheers, Daniel
> ---
>  xf86drm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index d900b4b..106b8ab 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -579,7 +579,7 @@ static int drmOpenByName(const char *name)
>  	if ((fd = open(proc_name, 0, 0)) >= 0) {
>  	    retcode = read(fd, buf, sizeof(buf)-1);
>  	    close(fd);
> -	    if (retcode) {
> +	    if (retcode > 0) {
>  		buf[retcode-1] = '\0';
>  		for (driver = pt = buf; *pt && *pt != ' '; ++pt)
>  		    ;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel