In theory it's possible for any of the nouveau_getparam calls to fail whist the last one being successful.
Thus at least one of the following (hard requirements) drmVersion, chipset and vram/gart memory size will be filled with garbage and sent to the userspace drivers.
Signed-off-by: Emil Velikov emil.l.velikov@gmail.com --- nouveau/nouveau.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c index ee7893b..d6013db 100644 --- a/nouveau/nouveau.c +++ b/nouveau/nouveau.c @@ -88,27 +88,32 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) nvdev->base.fd = fd;
ver = drmGetVersion(fd); - if (ver) dev->drm_version = (ver->version_major << 24) | - (ver->version_minor << 8) | - ver->version_patchlevel; + if (!ver) { + ret = -errno; + goto error; + } + + dev->drm_version = (ver->version_major << 24) | + (ver->version_minor << 8) | + ver->version_patchlevel; drmFreeVersion(ver);
if ( dev->drm_version != 0x00000010 && (dev->drm_version < 0x01000000 || dev->drm_version >= 0x02000000)) { - nouveau_device_del(&dev); - return -EINVAL; + ret = -EINVAL; + goto error; }
ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset); - if (ret == 0) + if (ret) + goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram); - if (ret == 0) + if (ret) + goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart); - if (ret) { - nouveau_device_del(&dev); - return ret; - } + if (ret) + goto error;
ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_HAS_BO_USAGE, &bousage); if (ret == 0) @@ -139,6 +144,9 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
*pdev = &nvdev->base; return 0; +error: + nouveau_device_del(&dev); + return -ret; }
int
Current handling relies on atoi which does not detect errors additionally, any integer value will be considered as a valid percent.
Resolve that by using strtol and limiting the value within the 5-100 (percent) range.
Signed-off-by: Emil Velikov emil.l.velikov@gmail.com --- nouveau/nouveau.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c index d6013db..06cd804 100644 --- a/nouveau/nouveau.c +++ b/nouveau/nouveau.c @@ -76,7 +76,7 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) struct nouveau_device *dev = &nvdev->base; uint64_t chipset, vram, gart, bousage; drmVersionPtr ver; - int ret; + int ret, limit; char *tmp;
#ifdef DEBUG @@ -121,16 +121,22 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
nvdev->close = close;
+ nvdev->vram_limit_percent = 80; tmp = getenv("NOUVEAU_LIBDRM_VRAM_LIMIT_PERCENT"); - if (tmp) - nvdev->vram_limit_percent = atoi(tmp); - else - nvdev->vram_limit_percent = 80; + if (tmp) { + limit = strtol(tmp, NULL, 10); + if (limit >= 5 && limit <= 100) + nvdev->vram_limit_percent = limit; + } + + nvdev->gart_limit_percent = 80; tmp = getenv("NOUVEAU_LIBDRM_GART_LIMIT_PERCENT"); - if (tmp) - nvdev->gart_limit_percent = atoi(tmp); - else - nvdev->gart_limit_percent = 80; + if (tmp) { + limit = strtol(tmp, NULL, 10); + if (limit >= 5 && limit <= 100) + nvdev->gart_limit_percent = limit; + } + DRMINITLISTHEAD(&nvdev->bo_list); nvdev->base.object.oclass = NOUVEAU_DEVICE_CLASS; nvdev->base.lib_version = 0x01000000;
On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
Current handling relies on atoi which does not detect errors additionally, any integer value will be considered as a valid percent.
Resolve that by using strtol and limiting the value within the 5-100 (percent) range.
Signed-off-by: Emil Velikov emil.l.velikov@gmail.com
nouveau/nouveau.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c index d6013db..06cd804 100644 --- a/nouveau/nouveau.c +++ b/nouveau/nouveau.c @@ -76,7 +76,7 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) struct nouveau_device *dev = &nvdev->base; uint64_t chipset, vram, gart, bousage; drmVersionPtr ver;
int ret;
int ret, limit; char *tmp;#ifdef DEBUG @@ -121,16 +121,22 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
nvdev->close = close;
nvdev->vram_limit_percent = 80; tmp = getenv("NOUVEAU_LIBDRM_VRAM_LIMIT_PERCENT");
if (tmp)nvdev->vram_limit_percent = atoi(tmp);elsenvdev->vram_limit_percent = 80;
if (tmp) {limit = strtol(tmp, NULL, 10);if (limit >= 5 && limit <= 100)nvdev->vram_limit_percent = limit;}
Wouldn't it be easier to just clamp the value no matter what? i.e. leave the current code alone, and add a clamp helper function + use it? These are pretty rare env vars to set... presumably the person setting them knows what they're doing. And if not, they get what they deserve.
On a related topic, I'd personally rather not throw in arbitrary restrictions to code like this -- it makes debugging harder later on. (e.g. what's wrong with 0 percent? let's say i want to disable gart/vram entirely?)
nvdev->gart_limit_percent = 80; tmp = getenv("NOUVEAU_LIBDRM_GART_LIMIT_PERCENT");
if (tmp)nvdev->gart_limit_percent = atoi(tmp);elsenvdev->gart_limit_percent = 80;
if (tmp) {limit = strtol(tmp, NULL, 10);if (limit >= 5 && limit <= 100)nvdev->gart_limit_percent = limit;}DRMINITLISTHEAD(&nvdev->bo_list); nvdev->base.object.oclass = NOUVEAU_DEVICE_CLASS; nvdev->base.lib_version = 0x01000000;-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 13/03/14 00:49, Ilia Mirkin wrote:
On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
Current handling relies on atoi which does not detect errors additionally, any integer value will be considered as a valid percent.
Resolve that by using strtol and limiting the value within the 5-100 (percent) range.
Signed-off-by: Emil Velikov emil.l.velikov@gmail.com
nouveau/nouveau.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c index d6013db..06cd804 100644 --- a/nouveau/nouveau.c +++ b/nouveau/nouveau.c @@ -76,7 +76,7 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) struct nouveau_device *dev = &nvdev->base; uint64_t chipset, vram, gart, bousage; drmVersionPtr ver;
int ret;
int ret, limit; char *tmp;#ifdef DEBUG
@@ -121,16 +121,22 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
nvdev->close = close;
nvdev->vram_limit_percent = 80; tmp = getenv("NOUVEAU_LIBDRM_VRAM_LIMIT_PERCENT");
if (tmp)nvdev->vram_limit_percent = atoi(tmp);elsenvdev->vram_limit_percent = 80;
if (tmp) {limit = strtol(tmp, NULL, 10);if (limit >= 5 && limit <= 100)nvdev->vram_limit_percent = limit;}Wouldn't it be easier to just clamp the value no matter what? i.e. leave the current code alone, and add a clamp helper function + use it? These are pretty rare env vars to set... presumably the person setting them knows what they're doing. And if not, they get what they deserve.
On a related topic, I'd personally rather not throw in arbitrary restrictions to code like this -- it makes debugging harder later on. (e.g. what's wrong with 0 percent? let's say i want to disable gart/vram entirely?)
Valid arguments, thanks.
-Emil
nvdev->gart_limit_percent = 80; tmp = getenv("NOUVEAU_LIBDRM_GART_LIMIT_PERCENT");
if (tmp)nvdev->gart_limit_percent = atoi(tmp);elsenvdev->gart_limit_percent = 80;
if (tmp) {limit = strtol(tmp, NULL, 10);if (limit >= 5 && limit <= 100)nvdev->gart_limit_percent = limit;}DRMINITLISTHEAD(&nvdev->bo_list); nvdev->base.object.oclass = NOUVEAU_DEVICE_CLASS; nvdev->base.lib_version = 0x01000000;-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Cc: Rob Clark robclark@freedesktop.org Signed-off-by: Emil Velikov emil.l.velikov@gmail.com --- freedreno/freedreno_device.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/freedreno/freedreno_device.c b/freedreno/freedreno_device.c index 598bdfb..c34963c 100644 --- a/freedreno/freedreno_device.c +++ b/freedreno/freedreno_device.c @@ -98,6 +98,7 @@ struct fd_device * fd_device_new(int fd) ERROR_MSG("unknown device: %s", version->name); dev = NULL; } + drmFreeVersion(version);
if (!dev) return NULL;
On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
Cc: Rob Clark robclark@freedesktop.org
Thanks,
Reviewed-by: Rob Clark robdclark@gmail.com
Signed-off-by: Emil Velikov emil.l.velikov@gmail.com
freedreno/freedreno_device.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/freedreno/freedreno_device.c b/freedreno/freedreno_device.c index 598bdfb..c34963c 100644 --- a/freedreno/freedreno_device.c +++ b/freedreno/freedreno_device.c @@ -98,6 +98,7 @@ struct fd_device * fd_device_new(int fd) ERROR_MSG("unknown device: %s", version->name); dev = NULL; }
drmFreeVersion(version); if (!dev) return NULL;-- 1.9.0
On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
In theory it's possible for any of the nouveau_getparam calls to fail whist the last one being successful.
Thus at least one of the following (hard requirements) drmVersion, chipset and vram/gart memory size will be filled with garbage and sent to the userspace drivers.
What was wrong with the old logic again? Except annoying indentation?
ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset); if (ret == 0) ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram); if (ret == 0) ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart); if (ret) { nouveau_device_del(&dev); return ret; }
It will only run each successive getparam if the previous one succeeded...
Signed-off-by: Emil Velikov emil.l.velikov@gmail.com
nouveau/nouveau.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c index ee7893b..d6013db 100644 --- a/nouveau/nouveau.c +++ b/nouveau/nouveau.c @@ -88,27 +88,32 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) nvdev->base.fd = fd;
ver = drmGetVersion(fd);
if (ver) dev->drm_version = (ver->version_major << 24) |(ver->version_minor << 8) |ver->version_patchlevel;
if (!ver) {ret = -errno;goto error;}dev->drm_version = (ver->version_major << 24) |(ver->version_minor << 8) |ver->version_patchlevel; drmFreeVersion(ver); if ( dev->drm_version != 0x00000010 && (dev->drm_version < 0x01000000 || dev->drm_version >= 0x02000000)) {
nouveau_device_del(&dev);return -EINVAL;
ret = -EINVAL;goto error; } ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
if (ret == 0)
if (ret)goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
if (ret == 0)
if (ret)goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
if (ret) {nouveau_device_del(&dev);return ret;}
if (ret)goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_HAS_BO_USAGE, &bousage); if (ret == 0)@@ -139,6 +144,9 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
*pdev = &nvdev->base; return 0;+error:
nouveau_device_del(&dev);return -ret;
you mean 'ret' of course.
}
int
1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 13/03/14 00:45, Ilia Mirkin wrote:
On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
In theory it's possible for any of the nouveau_getparam calls to fail whist the last one being successful.
Thus at least one of the following (hard requirements) drmVersion, chipset and vram/gart memory size will be filled with garbage and sent to the userspace drivers.
What was wrong with the old logic again? Except annoying indentation?
ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset); if (ret == 0) ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram); if (ret == 0) ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart); if (ret) { nouveau_device_del(&dev); return ret; }
It will only run each successive getparam if the previous one succeeded...
Good point, the lovely indentation got me. So it seems only !ver handling is needed.
Signed-off-by: Emil Velikov emil.l.velikov@gmail.com
nouveau/nouveau.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c index ee7893b..d6013db 100644 --- a/nouveau/nouveau.c +++ b/nouveau/nouveau.c @@ -88,27 +88,32 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) nvdev->base.fd = fd;
ver = drmGetVersion(fd);
if (ver) dev->drm_version = (ver->version_major << 24) |(ver->version_minor << 8) |ver->version_patchlevel;
if (!ver) {ret = -errno;goto error;}dev->drm_version = (ver->version_major << 24) |(ver->version_minor << 8) |ver->version_patchlevel; drmFreeVersion(ver); if ( dev->drm_version != 0x00000010 && (dev->drm_version < 0x01000000 || dev->drm_version >= 0x02000000)) {
nouveau_device_del(&dev);return -EINVAL;
ret = -EINVAL;goto error; } ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
if (ret == 0)
if (ret)goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
if (ret == 0)
if (ret)goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
if (ret) {nouveau_device_del(&dev);return ret;}
if (ret)goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_HAS_BO_USAGE, &bousage); if (ret == 0)@@ -139,6 +144,9 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
*pdev = &nvdev->base; return 0;+error:
nouveau_device_del(&dev);return -ret;you mean 'ret' of course.
Yikes, thanks.
-Emil
}
int
1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Mar 12, 2014 at 9:04 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
On 13/03/14 00:45, Ilia Mirkin wrote:
On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
In theory it's possible for any of the nouveau_getparam calls to fail whist the last one being successful.
Thus at least one of the following (hard requirements) drmVersion, chipset and vram/gart memory size will be filled with garbage and sent to the userspace drivers.
What was wrong with the old logic again? Except annoying indentation?
ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset); if (ret == 0) ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram); if (ret == 0) ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart); if (ret) { nouveau_device_del(&dev); return ret; }
It will only run each successive getparam if the previous one succeeded...
Good point, the lovely indentation got me. So it seems only !ver handling is needed.
Not really. drm->drm_version will be 0 if ver fails.
Signed-off-by: Emil Velikov emil.l.velikov@gmail.com
nouveau/nouveau.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c index ee7893b..d6013db 100644 --- a/nouveau/nouveau.c +++ b/nouveau/nouveau.c @@ -88,27 +88,32 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev) nvdev->base.fd = fd;
ver = drmGetVersion(fd);
if (ver) dev->drm_version = (ver->version_major << 24) |(ver->version_minor << 8) |ver->version_patchlevel;
if (!ver) {ret = -errno;goto error;}dev->drm_version = (ver->version_major << 24) |(ver->version_minor << 8) |ver->version_patchlevel; drmFreeVersion(ver); if ( dev->drm_version != 0x00000010 && (dev->drm_version < 0x01000000 || dev->drm_version >= 0x02000000)) {
nouveau_device_del(&dev);return -EINVAL;
ret = -EINVAL;goto error; } ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID,&chipset);
if (ret == 0)
if (ret)goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
if (ret == 0)
if (ret)goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
if (ret) {nouveau_device_del(&dev);return ret;}
if (ret)goto error; ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_HAS_BO_USAGE,&bousage); if (ret == 0) @@ -139,6 +144,9 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
*pdev = &nvdev->base; return 0;+error:
nouveau_device_del(&dev);return -ret;you mean 'ret' of course.
Yikes, thanks.
-Emil
}
int
1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 13/03/14 01:05, Ilia Mirkin wrote: [snip]
Not really. drm->drm_version will be 0 if ver fails.
Indeed, dev is calloc'ated by its callers, and if they mess around with that's their own fault.
Sorry for the noise. -Emil
On Wed, Mar 12, 2014 at 9:24 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
On 13/03/14 01:05, Ilia Mirkin wrote: [snip]
Not really. drm->drm_version will be 0 if ver fails.
Indeed, dev is calloc'ated by its callers, and if they mess around with that's their own fault.
The callers? It's calloc'd inside nouveau_device_wrap itself. Unless calloc is changed to not init to 0 (which would be a giant spec violation), I don't see how something could go wrong.
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Ilia Mirkin -
Rob Clark