[PATCH 5.10 0/1] drm/amdkfd: Check for null pointer after calling kmemdup

[PATCH 5.10 0/1] drm/amdkfd: Check for null pointer after calling kmemdup

[Dragos-Marian Panait]

The following commit is needed to fix CVE-2022-3108:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=abfaf0eee97925905e742aa3b0b72e04a918fa9e

Jiasheng Jiang (1):
  drm/amdkfd: Check for null pointer after calling kmemdup

 drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 3 +++
 1 file changed, 3 insertions(+)


base-commit: 0fe4548663f7dc2c3b549ef54ce9bb6d40a235be
-- 
2.38.1

[PATCH 5.10 1/1] drm/amdkfd: Check for null pointer after calling kmemdup

[Dragos-Marian Panait]

From: Jiasheng Jiang <jiasheng@iscas.ac.cn>

[ Upstream commit abfaf0eee97925905e742aa3b0b72e04a918fa9e ]

As the possible failure of the allocation, kmemdup() may return NULL
pointer.
Therefore, it should be better to check the 'props2' in order to prevent
the dereference of NULL pointer.

Fixes: 3a87177eb141 ("drm/amdkfd: Add topology support for dGPUs")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Dragos-Marian Panait <dragos.panait@windriver.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index 86b4dadf772e..02e3c650ed1c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -408,6 +408,9 @@ static int kfd_parse_subtype_iolink(struct crat_subtype_iolink *iolink,
 			return -ENODEV;
 		/* same everything but the other direction */
 		props2 = kmemdup(props, sizeof(*props2), GFP_KERNEL);
+		if (!props2)
+			return -ENOMEM;
+
 		props2->node_from = id_to;
 		props2->node_to = id_from;
 		props2->kobj = NULL;
-- 
2.38.1

Re: [PATCH 5.10 1/1] drm/amdkfd: Check for null pointer after calling kmemdup

[Greg KH]

On Wed, Jan 04, 2023 at 07:56:33PM +0200, Dragos-Marian Panait wrote:
> From: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> 
> [ Upstream commit abfaf0eee97925905e742aa3b0b72e04a918fa9e ]
> 
> As the possible failure of the allocation, kmemdup() may return NULL
> pointer.
> Therefore, it should be better to check the 'props2' in order to prevent
> the dereference of NULL pointer.
> 
> Fixes: 3a87177eb141 ("drm/amdkfd: Add topology support for dGPUs")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Dragos-Marian Panait <dragos.panait@windriver.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> index 86b4dadf772e..02e3c650ed1c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> @@ -408,6 +408,9 @@ static int kfd_parse_subtype_iolink(struct crat_subtype_iolink *iolink,
>  			return -ENODEV;
>  		/* same everything but the other direction */
>  		props2 = kmemdup(props, sizeof(*props2), GFP_KERNEL);
> +		if (!props2)
> +			return -ENOMEM;

Not going to queue this up as this is a bogus CVE.

Re: [PATCH 5.10 1/1] drm/amdkfd: Check for null pointer after calling kmemdup

[Daniel Vetter]

On Thu, 12 Jan 2023 at 13:47, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 04, 2023 at 07:56:33PM +0200, Dragos-Marian Panait wrote:
> > From: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> >
> > [ Upstream commit abfaf0eee97925905e742aa3b0b72e04a918fa9e ]
> >
> > As the possible failure of the allocation, kmemdup() may return NULL
> > pointer.
> > Therefore, it should be better to check the 'props2' in order to prevent
> > the dereference of NULL pointer.
> >
> > Fixes: 3a87177eb141 ("drm/amdkfd: Add topology support for dGPUs")
> > Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> > Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > Signed-off-by: Dragos-Marian Panait <dragos.panait@windriver.com>
> > ---
> >  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > index 86b4dadf772e..02e3c650ed1c 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > @@ -408,6 +408,9 @@ static int kfd_parse_subtype_iolink(struct crat_subtype_iolink *iolink,
> >                       return -ENODEV;
> >               /* same everything but the other direction */
> >               props2 = kmemdup(props, sizeof(*props2), GFP_KERNEL);
> > +             if (!props2)
> > +                     return -ENOMEM;
>
> Not going to queue this up as this is a bogus CVE.

Are we at the point where CVE presence actually contraindicates
backporting? At least I'm getting a bit the feeling there's a surge of
automated (security) fixes that just don't hold up to any scrutiny.
Last week I had to toss out an fbdev locking patch due to static
checker that has no clue at all how refcounting works, and so
complained that things need more locking ... (that was -fixes, but
would probably have gone to stable too if I didn't catch it).

Simple bugfixes from random people was nice when it was checkpatch
stuff and I was fairly happy to take these aggressively in drm. But my
gut feeling says things seem to be shifting towards more advanced
tooling, but without more advanced understanding by submitters. Does
that holder in other areas too?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 5.10 1/1] drm/amdkfd: Check for null pointer after calling kmemdup

[Greg KH]

On Thu, Jan 12, 2023 at 04:26:45PM +0100, Daniel Vetter wrote:
> On Thu, 12 Jan 2023 at 13:47, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jan 04, 2023 at 07:56:33PM +0200, Dragos-Marian Panait wrote:
> > > From: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> > >
> > > [ Upstream commit abfaf0eee97925905e742aa3b0b72e04a918fa9e ]
> > >
> > > As the possible failure of the allocation, kmemdup() may return NULL
> > > pointer.
> > > Therefore, it should be better to check the 'props2' in order to prevent
> > > the dereference of NULL pointer.
> > >
> > > Fixes: 3a87177eb141 ("drm/amdkfd: Add topology support for dGPUs")
> > > Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> > > Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> > > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > > Signed-off-by: Dragos-Marian Panait <dragos.panait@windriver.com>
> > > ---
> > >  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > > index 86b4dadf772e..02e3c650ed1c 100644
> > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > > @@ -408,6 +408,9 @@ static int kfd_parse_subtype_iolink(struct crat_subtype_iolink *iolink,
> > >                       return -ENODEV;
> > >               /* same everything but the other direction */
> > >               props2 = kmemdup(props, sizeof(*props2), GFP_KERNEL);
> > > +             if (!props2)
> > > +                     return -ENOMEM;
> >
> > Not going to queue this up as this is a bogus CVE.
> 
> Are we at the point where CVE presence actually contraindicates
> backporting?

Some would say that that point passed a long time ago :)

> At least I'm getting a bit the feeling there's a surge of
> automated (security) fixes that just don't hold up to any scrutiny.

That has been happening a lot more in the past 6-8 months than in years
past with the introduction of more automated tools being present.

> Last week I had to toss out an fbdev locking patch due to static
> checker that has no clue at all how refcounting works, and so
> complained that things need more locking ... (that was -fixes, but
> would probably have gone to stable too if I didn't catch it).
> 
> Simple bugfixes from random people was nice when it was checkpatch
> stuff and I was fairly happy to take these aggressively in drm. But my
> gut feeling says things seem to be shifting towards more advanced
> tooling, but without more advanced understanding by submitters. Does
> that holder in other areas too?

Again, yes, I have seen that a lot recently, especially with regards to
patches that purport to fix bugs yet obviously were never tested.

That being said, there are a few developers who are doing great things
with fault-injection testing and providing good patches for that.  So we
can't just say that everyone using these tools has no clue.

thanks,

greg k-h

Re: [PATCH 5.10 1/1] drm/amdkfd: Check for null pointer after calling kmemdup

[Daniel Vetter]

On Thu, Jan 12, 2023 at 05:45:42PM +0100, Greg KH wrote:
> On Thu, Jan 12, 2023 at 04:26:45PM +0100, Daniel Vetter wrote:
> > On Thu, 12 Jan 2023 at 13:47, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Wed, Jan 04, 2023 at 07:56:33PM +0200, Dragos-Marian Panait wrote:
> > > > From: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> > > >
> > > > [ Upstream commit abfaf0eee97925905e742aa3b0b72e04a918fa9e ]
> > > >
> > > > As the possible failure of the allocation, kmemdup() may return NULL
> > > > pointer.
> > > > Therefore, it should be better to check the 'props2' in order to prevent
> > > > the dereference of NULL pointer.
> > > >
> > > > Fixes: 3a87177eb141 ("drm/amdkfd: Add topology support for dGPUs")
> > > > Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> > > > Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> > > > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> > > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > > > Signed-off-by: Dragos-Marian Panait <dragos.panait@windriver.com>
> > > > ---
> > > >  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > > > index 86b4dadf772e..02e3c650ed1c 100644
> > > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> > > > @@ -408,6 +408,9 @@ static int kfd_parse_subtype_iolink(struct crat_subtype_iolink *iolink,
> > > >                       return -ENODEV;
> > > >               /* same everything but the other direction */
> > > >               props2 = kmemdup(props, sizeof(*props2), GFP_KERNEL);
> > > > +             if (!props2)
> > > > +                     return -ENOMEM;
> > >
> > > Not going to queue this up as this is a bogus CVE.
> > 
> > Are we at the point where CVE presence actually contraindicates
> > backporting?
> 
> Some would say that that point passed a long time ago :)
> 
> > At least I'm getting a bit the feeling there's a surge of
> > automated (security) fixes that just don't hold up to any scrutiny.
> 
> That has been happening a lot more in the past 6-8 months than in years
> past with the introduction of more automated tools being present.

Ok, gut feeling confirmed, I'll try and keep more a lookout for these.

I guess next step is that people will use chatgpt to write the patches for
these bugs.

> > Last week I had to toss out an fbdev locking patch due to static
> > checker that has no clue at all how refcounting works, and so
> > complained that things need more locking ... (that was -fixes, but
> > would probably have gone to stable too if I didn't catch it).
> > 
> > Simple bugfixes from random people was nice when it was checkpatch
> > stuff and I was fairly happy to take these aggressively in drm. But my
> > gut feeling says things seem to be shifting towards more advanced
> > tooling, but without more advanced understanding by submitters. Does
> > that holder in other areas too?
> 
> Again, yes, I have seen that a lot recently, especially with regards to
> patches that purport to fix bugs yet obviously were never tested.
> 
> That being said, there are a few developers who are doing great things
> with fault-injection testing and providing good patches for that.  So we
> can't just say that everyone using these tools has no clue.

Oh yes there's definitely awesome stuff happening, which is why I do not
want to throw them all out. And waiting until the name is recognizeable
for individual maintainers like me that don't see the entire fixes flood
is also not really an approach.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 5.10 1/1] drm/amdkfd: Check for null pointer after calling kmemdup

[Christian König]

Am 19.01.23 um 11:26 schrieb Daniel Vetter:
> [SNIP]
> I guess next step is that people will use chatgpt to write the patches for
> these bugs.

To be honest I think that would result in quite some improvement in the 
average patch quality.

That guessing this AI does has at least a statistically proven chance to 
be correct.

Cheers,
Christian.