| Message ID | 1394657145-2638-1-git-send-email-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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(-)