* [PATCH] drm/managed: Fix off-by-one in warning
@ 2020-03-28 16:23 Daniel Vetter
2020-03-28 18:49 ` Sam Ravnborg
2020-03-30 9:29 ` Daniel Vetter
0 siblings, 2 replies; 6+ messages in thread
From: Daniel Vetter @ 2020-03-28 16:23 UTC (permalink / raw)
To: DRI Development
Cc: kernel test robot, Rafael J. Wysocki, Daniel Vetter,
Laurent Pinchart, Thomas Zimmermann, Greg Kroah-Hartman,
Daniel Vetter, Sam Ravnborg, Dan Carpenter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/managed: Fix off-by-one in warning
2020-03-28 16:23 [PATCH] drm/managed: Fix off-by-one in warning Daniel Vetter
@ 2020-03-28 18:49 ` Sam Ravnborg
2020-03-28 22:02 ` Daniel Vetter
2020-03-30 9:29 ` Daniel Vetter
1 sibling, 1 reply; 6+ messages in thread
From: Sam Ravnborg @ 2020-03-28 18:49 UTC (permalink / raw)
To: Daniel Vetter
Cc: kernel test robot, Rafael J. Wysocki, Greg Kroah-Hartman,
DRI Development, Laurent Pinchart, Thomas Zimmermann,
Daniel Vetter, Dan Carpenter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/managed: Fix off-by-one in warning
2020-03-28 18:49 ` Sam Ravnborg
@ 2020-03-28 22:02 ` Daniel Vetter
2020-03-30 10:29 ` Sam Ravnborg
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2020-03-28 22:02 UTC (permalink / raw)
To: Sam Ravnborg
Cc: kernel test robot, Rafael J. Wysocki, Greg Kroah-Hartman,
DRI Development, Laurent Pinchart, Thomas Zimmermann,
Daniel Vetter, Dan Carpenter
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
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/managed: Fix off-by-one in warning
2020-03-28 16:23 [PATCH] drm/managed: Fix off-by-one in warning Daniel Vetter
2020-03-28 18:49 ` Sam Ravnborg
@ 2020-03-30 9:29 ` Daniel Vetter
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2020-03-30 9:29 UTC (permalink / raw)
To: DRI Development
Cc: kernel test robot, Rafael J. Wysocki, Greg Kroah-Hartman,
Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Sam Ravnborg,
Dan Carpenter
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
>
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/managed: Fix off-by-one in warning
2020-03-28 22:02 ` Daniel Vetter
@ 2020-03-30 10:29 ` Sam Ravnborg
2020-03-30 10:45 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Sam Ravnborg @ 2020-03-30 10:29 UTC (permalink / raw)
To: Daniel Vetter
Cc: kernel test robot, Rafael J. Wysocki, Greg Kroah-Hartman,
DRI Development, Laurent Pinchart, Thomas Zimmermann,
Daniel Vetter, Dan Carpenter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/managed: Fix off-by-one in warning
2020-03-30 10:29 ` Sam Ravnborg
@ 2020-03-30 10:45 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2020-03-30 10:45 UTC (permalink / raw)
To: Sam Ravnborg
Cc: kernel test robot, Rafael J. Wysocki, Daniel Vetter,
DRI Development, Laurent Pinchart, Thomas Zimmermann,
Greg Kroah-Hartman, Daniel Vetter, Dan Carpenter
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
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-30 10:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-28 16:23 [PATCH] drm/managed: Fix off-by-one in warning Daniel Vetter
2020-03-28 18:49 ` Sam Ravnborg
2020-03-28 22:02 ` Daniel Vetter
2020-03-30 10:29 ` Sam Ravnborg
2020-03-30 10:45 ` Daniel Vetter
2020-03-30 9:29 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.