| Message ID | 20200328162358.18500-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/managed: Fix off-by-one in warning | expand |
Hi Daniel. On Sat, Mar 28, 2020 at 05:23:58PM +0100, Daniel Vetter wrote: > I'm thinking this is the warning that fired in the 0day report, but I > can't double-check yet since 0day didn't upload its source tree > anywhere I can check. And all the drivers I can easily test don't use > drm_dev_alloc anymore ... > > Also if I'm correct supreme amounts of bad luck because usually kslap > (for bigger structures) gives us something quite a bit bigger than > what we asked for. > > Reported-by: kernel test robot <lkp@intel.com> > Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Dan Carpenter <dan.carpenter@oracle.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Neil Armstrong <narmstrong@baylibre.com > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_managed.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > index 4955241ceb4c..9cebfe370a65 100644 > --- a/drivers/gpu/drm/drm_managed.c > +++ b/drivers/gpu/drm/drm_managed.c > @@ -139,8 +139,7 @@ void drmm_add_final_kfree(struct drm_device *dev, void *container) > { > WARN_ON(dev->managed.final_kfree); > WARN_ON(dev < (struct drm_device *) container); > - WARN_ON(dev + 1 >= > - (struct drm_device *) (container + ksize(container))); > + WARN_ON(dev + 1 > (struct drm_device *) (container + ksize(container))); I do not think this is the right fix... The original code would trigger if 1) the container only had a drm_device - and nothing else 2) and the allocated size was the same And the modification will now allow for a container with the exact size of drm_device. I checked all users in my tree - no-one only had a drm_device. The minimum was one extra pointer. Another thing that could trigger the warning was if any users did not specify a pointer to memory allocated by k(z)alloc() But I could not find any. tiny/st7735r.c looked suspisius, but I think it is also OK, because struct st7735r_priv is allocated, but the poitner specified in st7735r_priv.dbidev. But dbidev is the first field - so OK. So no better clue... Sam
On Sat, Mar 28, 2020 at 7:49 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Daniel. > > On Sat, Mar 28, 2020 at 05:23:58PM +0100, Daniel Vetter wrote: > > I'm thinking this is the warning that fired in the 0day report, but I > > can't double-check yet since 0day didn't upload its source tree > > anywhere I can check. And all the drivers I can easily test don't use > > drm_dev_alloc anymore ... > > > > Also if I'm correct supreme amounts of bad luck because usually kslap > > (for bigger structures) gives us something quite a bit bigger than > > what we asked for. > > > > Reported-by: kernel test robot <lkp@intel.com> > > Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: Dan Carpenter <dan.carpenter@oracle.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Neil Armstrong <narmstrong@baylibre.com > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_managed.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > > index 4955241ceb4c..9cebfe370a65 100644 > > --- a/drivers/gpu/drm/drm_managed.c > > +++ b/drivers/gpu/drm/drm_managed.c > > @@ -139,8 +139,7 @@ void drmm_add_final_kfree(struct drm_device *dev, void *container) > > { > > WARN_ON(dev->managed.final_kfree); > > WARN_ON(dev < (struct drm_device *) container); > > - WARN_ON(dev + 1 >= > > - (struct drm_device *) (container + ksize(container))); > > + WARN_ON(dev + 1 > (struct drm_device *) (container + ksize(container))); > > I do not think this is the right fix... > The original code would trigger if > 1) the container only had a drm_device - and nothing else > 2) and the allocated size was the same Yup, which apparently happens for all the drivers calling drm_dev_alloc(). At least on the unlucky architecture that 0day tested on (or build settings, or whatever). The issue was hit with drm/bochs, which is still using drm_dev_alloc (like most older-ish drivers). > And the modification will now allow for a container with the exact size > of drm_device. > > I checked all users in my tree - no-one only had a drm_device. > The minimum was one extra pointer. > > Another thing that could trigger the warning was if any users > did not specify a pointer to memory allocated by k(z)alloc() > But I could not find any. > > tiny/st7735r.c looked suspisius, but I think it is also OK, > because struct st7735r_priv is allocated, but the poitner specified in > st7735r_priv.dbidev. But dbidev is the first field - so OK. > > So no better clue... Yeah all the drivers using drm_dev_init with embedded drm_device wont hit this. -Daniel
Ok 0day people uploaded the tree they tested (from patches) now, I confirmed it's indeed this line that's blowing up. -Daniel On Sat, Mar 28, 2020 at 5:24 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > I'm thinking this is the warning that fired in the 0day report, but I > can't double-check yet since 0day didn't upload its source tree > anywhere I can check. And all the drivers I can easily test don't use > drm_dev_alloc anymore ... > > Also if I'm correct supreme amounts of bad luck because usually kslap > (for bigger structures) gives us something quite a bit bigger than > what we asked for. > > Reported-by: kernel test robot <lkp@intel.com> > Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Dan Carpenter <dan.carpenter@oracle.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Neil Armstrong <narmstrong@baylibre.com > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_managed.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > index 4955241ceb4c..9cebfe370a65 100644 > --- a/drivers/gpu/drm/drm_managed.c > +++ b/drivers/gpu/drm/drm_managed.c > @@ -139,8 +139,7 @@ void drmm_add_final_kfree(struct drm_device *dev, void *container) > { > WARN_ON(dev->managed.final_kfree); > WARN_ON(dev < (struct drm_device *) container); > - WARN_ON(dev + 1 >= > - (struct drm_device *) (container + ksize(container))); > + WARN_ON(dev + 1 > (struct drm_device *) (container + ksize(container))); > dev->managed.final_kfree = container; > } > EXPORT_SYMBOL(drmm_add_final_kfree); > -- > 2.25.1 >
On Sat, Mar 28, 2020 at 11:02:26PM +0100, Daniel Vetter wrote: > On Sat, Mar 28, 2020 at 7:49 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > Hi Daniel. > > > > On Sat, Mar 28, 2020 at 05:23:58PM +0100, Daniel Vetter wrote: > > > I'm thinking this is the warning that fired in the 0day report, but I > > > can't double-check yet since 0day didn't upload its source tree > > > anywhere I can check. And all the drivers I can easily test don't use > > > drm_dev_alloc anymore ... > > > > > > Also if I'm correct supreme amounts of bad luck because usually kslap > > > (for bigger structures) gives us something quite a bit bigger than > > > what we asked for. > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > Cc: Dan Carpenter <dan.carpenter@oracle.com> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Cc: Neil Armstrong <narmstrong@baylibre.com > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > drivers/gpu/drm/drm_managed.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > > > index 4955241ceb4c..9cebfe370a65 100644 > > > --- a/drivers/gpu/drm/drm_managed.c > > > +++ b/drivers/gpu/drm/drm_managed.c > > > @@ -139,8 +139,7 @@ void drmm_add_final_kfree(struct drm_device *dev, void *container) > > > { > > > WARN_ON(dev->managed.final_kfree); > > > WARN_ON(dev < (struct drm_device *) container); > > > - WARN_ON(dev + 1 >= > > > - (struct drm_device *) (container + ksize(container))); > > > + WARN_ON(dev + 1 > (struct drm_device *) (container + ksize(container))); > > > > I do not think this is the right fix... > > The original code would trigger if > > 1) the container only had a drm_device - and nothing else > > 2) and the allocated size was the same > > Yup, which apparently happens for all the drivers calling > drm_dev_alloc(). At least on the unlucky architecture that 0day tested > on (or build settings, or whatever). The issue was hit with drm/bochs, > which is still using drm_dev_alloc (like most older-ish drivers). That explains it and then the checks makes sense. Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Sam
On Mon, Mar 30, 2020 at 12:29:44PM +0200, Sam Ravnborg wrote: > On Sat, Mar 28, 2020 at 11:02:26PM +0100, Daniel Vetter wrote: > > On Sat, Mar 28, 2020 at 7:49 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > > Hi Daniel. > > > > > > On Sat, Mar 28, 2020 at 05:23:58PM +0100, Daniel Vetter wrote: > > > > I'm thinking this is the warning that fired in the 0day report, but I > > > > can't double-check yet since 0day didn't upload its source tree > > > > anywhere I can check. And all the drivers I can easily test don't use > > > > drm_dev_alloc anymore ... > > > > > > > > Also if I'm correct supreme amounts of bad luck because usually kslap > > > > (for bigger structures) gives us something quite a bit bigger than > > > > what we asked for. > > > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") > > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > Cc: Dan Carpenter <dan.carpenter@oracle.com> > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Cc: Neil Armstrong <narmstrong@baylibre.com > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > --- > > > > drivers/gpu/drm/drm_managed.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > > > > index 4955241ceb4c..9cebfe370a65 100644 > > > > --- a/drivers/gpu/drm/drm_managed.c > > > > +++ b/drivers/gpu/drm/drm_managed.c > > > > @@ -139,8 +139,7 @@ void drmm_add_final_kfree(struct drm_device *dev, void *container) > > > > { > > > > WARN_ON(dev->managed.final_kfree); > > > > WARN_ON(dev < (struct drm_device *) container); > > > > - WARN_ON(dev + 1 >= > > > > - (struct drm_device *) (container + ksize(container))); > > > > + WARN_ON(dev + 1 > (struct drm_device *) (container + ksize(container))); > > > > > > I do not think this is the right fix... > > > The original code would trigger if > > > 1) the container only had a drm_device - and nothing else > > > 2) and the allocated size was the same > > > > Yup, which apparently happens for all the drivers calling > > drm_dev_alloc(). At least on the unlucky architecture that 0day tested > > on (or build settings, or whatever). The issue was hit with drm/bochs, > > which is still using drm_dev_alloc (like most older-ish drivers). > > That explains it and then the checks makes sense. > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Thanks for your review, patch applied and a note to the commit message added that 0day confirmed that it's indeed this WARN_ON that they've hit. -Daniel
diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c index 4955241ceb4c..9cebfe370a65 100644 --- a/drivers/gpu/drm/drm_managed.c +++ b/drivers/gpu/drm/drm_managed.c @@ -139,8 +139,7 @@ void drmm_add_final_kfree(struct drm_device *dev, void *container) { WARN_ON(dev->managed.final_kfree); WARN_ON(dev < (struct drm_device *) container); - WARN_ON(dev + 1 >= - (struct drm_device *) (container + ksize(container))); + WARN_ON(dev + 1 > (struct drm_device *) (container + ksize(container))); dev->managed.final_kfree = container; } EXPORT_SYMBOL(drmm_add_final_kfree);
I'm thinking this is the warning that fired in the 0day report, but I can't double-check yet since 0day didn't upload its source tree anywhere I can check. And all the drivers I can easily test don't use drm_dev_alloc anymore ... Also if I'm correct supreme amounts of bad luck because usually kslap (for bigger structures) gives us something quite a bit bigger than what we asked for. Reported-by: kernel test robot <lkp@intel.com> Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Dan Carpenter <dan.carpenter@oracle.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Neil Armstrong <narmstrong@baylibre.com Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_managed.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)