Message ID | 1393000925-8446-2-git-send-email-armbru@redhat.com |
---|---|
State | New |
Headers | show |
On 21 February 2014 16:42, Markus Armbruster <armbru@redhat.com> wrote: > readlink() returns the number of bytes written to the buffer, and it > doesn't write a terminating null byte. vfio_init() writes it itself. > Overruns the buffer when readlink() filled it completely. > > Fix by reserving space for the null byte when calling readlink(), like > we do elsewhere. > > Spotted by Coverity. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/misc/vfio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c > index 8db182f..8e56785 100644 > --- a/hw/misc/vfio.c > +++ b/hw/misc/vfio.c > @@ -3681,7 +3681,7 @@ static int vfio_initfn(PCIDevice *pdev) > > strncat(path, "iommu_group", sizeof(path) - strlen(path) - 1); > > - len = readlink(path, iommu_group_path, PATH_MAX); > + len = readlink(path, iommu_group_path, PATH_MAX - 1); > if (len <= 0) { > error_report("vfio: error no iommu_group for device"); > return -errno; "sizeof(iommu_group_path) - 1" would be slightly better, I think, but PATH_MAX - 1 works too. Reviewed-by: Peter Maydell <peter.maydell@linaro.org> (cc stable?) thanks -- PMM
On Fri, 2014-02-21 at 17:42 +0100, Markus Armbruster wrote: > readlink() returns the number of bytes written to the buffer, and it > doesn't write a terminating null byte. vfio_init() writes it itself. > Overruns the buffer when readlink() filled it completely. > > Fix by reserving space for the null byte when calling readlink(), like > we do elsewhere. > > Spotted by Coverity. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/misc/vfio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c > index 8db182f..8e56785 100644 > --- a/hw/misc/vfio.c > +++ b/hw/misc/vfio.c > @@ -3681,7 +3681,7 @@ static int vfio_initfn(PCIDevice *pdev) > > strncat(path, "iommu_group", sizeof(path) - strlen(path) - 1); > > - len = readlink(path, iommu_group_path, PATH_MAX); > + len = readlink(path, iommu_group_path, PATH_MAX - 1); > if (len <= 0) { > error_report("vfio: error no iommu_group for device"); > return -errno; I'm not sure why we wouldn't follow the same logic as pci-assign here. If we fill to the length provided we have no idea if we have the full path. We certainly expect it to fit well within PATH_MAX, so let's handle it as an error if we reach PATH_MAX. Thanks, Alex
Alex Williamson <alex.williamson@redhat.com> writes: > On Fri, 2014-02-21 at 17:42 +0100, Markus Armbruster wrote: >> readlink() returns the number of bytes written to the buffer, and it >> doesn't write a terminating null byte. vfio_init() writes it itself. >> Overruns the buffer when readlink() filled it completely. >> >> Fix by reserving space for the null byte when calling readlink(), like >> we do elsewhere. >> >> Spotted by Coverity. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> hw/misc/vfio.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c >> index 8db182f..8e56785 100644 >> --- a/hw/misc/vfio.c >> +++ b/hw/misc/vfio.c >> @@ -3681,7 +3681,7 @@ static int vfio_initfn(PCIDevice *pdev) >> >> strncat(path, "iommu_group", sizeof(path) - strlen(path) - 1); >> >> - len = readlink(path, iommu_group_path, PATH_MAX); >> + len = readlink(path, iommu_group_path, PATH_MAX - 1); >> if (len <= 0) { >> error_report("vfio: error no iommu_group for device"); >> return -errno; > > I'm not sure why we wouldn't follow the same logic as pci-assign here. > If we fill to the length provided we have no idea if we have the full > path. We certainly expect it to fit well within PATH_MAX, so let's > handle it as an error if we reach PATH_MAX. Thanks, Fair point. v2 sent.
diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c index 8db182f..8e56785 100644 --- a/hw/misc/vfio.c +++ b/hw/misc/vfio.c @@ -3681,7 +3681,7 @@ static int vfio_initfn(PCIDevice *pdev) strncat(path, "iommu_group", sizeof(path) - strlen(path) - 1); - len = readlink(path, iommu_group_path, PATH_MAX); + len = readlink(path, iommu_group_path, PATH_MAX - 1); if (len <= 0) { error_report("vfio: error no iommu_group for device"); return -errno;
readlink() returns the number of bytes written to the buffer, and it doesn't write a terminating null byte. vfio_init() writes it itself. Overruns the buffer when readlink() filled it completely. Fix by reserving space for the null byte when calling readlink(), like we do elsewhere. Spotted by Coverity. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/misc/vfio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)