Re: [PATCH] drm/managed: Fix off-by-one in warning
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 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 > > > > Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") > > > > Cc: Sam Ravnborg > > > > Cc: Thomas Zimmermann > > > > Cc: Dan Carpenter > > > > Cc: Laurent Pinchart > > > > Cc: Neil Armstrong > > > Cc: Greg Kroah-Hartman > > > > Cc: "Rafael J. Wysocki" > > > > Signed-off-by: Daniel Vetter > > > > --- > > > > 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 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/managed: Fix off-by-one in warning
On Sat, Mar 28, 2020 at 11:02:26PM +0100, Daniel Vetter wrote: > On Sat, Mar 28, 2020 at 7:49 PM Sam Ravnborg 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 > > > Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") > > > Cc: Sam Ravnborg > > > Cc: Thomas Zimmermann > > > Cc: Dan Carpenter > > > Cc: Laurent Pinchart > > > Cc: Neil Armstrong > > Cc: Greg Kroah-Hartman > > > Cc: "Rafael J. Wysocki" > > > Signed-off-by: Daniel Vetter > > > --- > > > 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/managed: Fix off-by-one in warning
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 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 > Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > Cc: Dan Carpenter > Cc: Laurent Pinchart > Cc: Neil Armstrong Cc: Greg Kroah-Hartman > Cc: "Rafael J. Wysocki" > Signed-off-by: Daniel Vetter > --- > 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 > -- 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 https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/managed: Fix off-by-one in warning
On Sat, Mar 28, 2020 at 7:49 PM Sam Ravnborg 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 > > Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") > > Cc: Sam Ravnborg > > Cc: Thomas Zimmermann > > Cc: Dan Carpenter > > Cc: Laurent Pinchart > > Cc: Neil Armstrong > Cc: Greg Kroah-Hartman > > Cc: "Rafael J. Wysocki" > > Signed-off-by: Daniel Vetter > > --- > > 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 -- 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 https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/managed: Fix off-by-one in warning
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 > Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > Cc: Dan Carpenter > Cc: Laurent Pinchart > Cc: Neil Armstrong Cc: Greg Kroah-Hartman > Cc: "Rafael J. Wysocki" > Signed-off-by: Daniel Vetter > --- > 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/managed: Fix off-by-one in warning
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 Fixes: c6603c740e0e ("drm: add managed resources tied to drm_device") Cc: Sam Ravnborg Cc: Thomas Zimmermann Cc: Dan Carpenter Cc: Laurent Pinchart Cc: Neil Armstrong Cc: "Rafael J. Wysocki" Signed-off-by: Daniel Vetter --- 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel