[PATCH] thunderbolt: handle possible NULL pointer from get_device()

[PATCH] thunderbolt: handle possible NULL pointer from get_device()

[Dmitry Antipov]

Handle possible NULL pointer returned by 'get_device()'
in 'tb_xdomain_alloc()' and 'remove_unplugged_switch()'.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/thunderbolt/icm.c     | 5 +++++
 drivers/thunderbolt/xdomain.c | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 86521ebb2579..40ab6104a437 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -2035,6 +2035,11 @@ static void remove_unplugged_switch(struct tb_switch *sw)
 {
 	struct device *parent = get_device(sw->dev.parent);
 
+	if (!parent) {
+		tb_warn(sw->tb, "no parent of switch %pUb\n", sw->uuid);
+		return;
+	}
+
 	pm_runtime_get_sync(parent);
 
 	/*
diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index e2b54887d331..a0ee683d752e 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -1883,6 +1883,8 @@ struct tb_xdomain *tb_xdomain_alloc(struct tb *tb, struct device *parent,
 
 	device_initialize(&xd->dev);
 	xd->dev.parent = get_device(parent);
+	if (!xd->dev.parent)
+		goto err_free_remote_uuid;
 	xd->dev.bus = &tb_bus_type;
 	xd->dev.type = &tb_xdomain_type;
 	xd->dev.groups = xdomain_attr_groups;
@@ -1902,6 +1904,8 @@ struct tb_xdomain *tb_xdomain_alloc(struct tb *tb, struct device *parent,
 
 	return xd;
 
+err_free_remote_uuid:
+	kfree(xd->remote_uuid);
 err_free_local_uuid:
 	kfree(xd->local_uuid);
 err_free:
-- 
2.40.1

Re: [PATCH] thunderbolt: handle possible NULL pointer from get_device()

[Greg KH]

On Fri, Jun 09, 2023 at 09:16:19AM +0300, Dmitry Antipov wrote:
> Handle possible NULL pointer returned by 'get_device()'
> in 'tb_xdomain_alloc()' and 'remove_unplugged_switch()'.

Sorry, but how can that happen?

> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/thunderbolt/icm.c     | 5 +++++
>  drivers/thunderbolt/xdomain.c | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
> index 86521ebb2579..40ab6104a437 100644
> --- a/drivers/thunderbolt/icm.c
> +++ b/drivers/thunderbolt/icm.c
> @@ -2035,6 +2035,11 @@ static void remove_unplugged_switch(struct tb_switch *sw)
>  {
>  	struct device *parent = get_device(sw->dev.parent);
>  
> +	if (!parent) {

This will never fail, how did you test this?


> +		tb_warn(sw->tb, "no parent of switch %pUb\n", sw->uuid);
> +		return;
> +	}
> +
>  	pm_runtime_get_sync(parent);
>  
>  	/*
> diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
> index e2b54887d331..a0ee683d752e 100644
> --- a/drivers/thunderbolt/xdomain.c
> +++ b/drivers/thunderbolt/xdomain.c
> @@ -1883,6 +1883,8 @@ struct tb_xdomain *tb_xdomain_alloc(struct tb *tb, struct device *parent,
>  
>  	device_initialize(&xd->dev);
>  	xd->dev.parent = get_device(parent);
> +	if (!xd->dev.parent)

Again, how did you test this?  How can that ever happen?

thanks,

greg k-h

Re: [PATCH] thunderbolt: handle possible NULL pointer from get_device()

[Dmitry Antipov]

On 6/9/23 09:30, Greg KH wrote:

> Sorry, but how can that happen?

Hmmm.. there should be a complete mess in the device tree (e.g. passing
root device with no parent where the regular device is expected). If
you're sure that this doesn't worth checking then please disregard this.

Dmitry

Re: [PATCH] thunderbolt: handle possible NULL pointer from get_device()

[Greg KH]

On Fri, Jun 09, 2023 at 10:15:36AM +0300, Dmitry Antipov wrote:
> On 6/9/23 09:30, Greg KH wrote:
> 
> > Sorry, but how can that happen?
> 
> Hmmm.. there should be a complete mess in the device tree (e.g. passing
> root device with no parent where the regular device is expected). If
> you're sure that this doesn't worth checking then please disregard this.

Again, how did you test this?

And yes, the driver model guarantees that a child can never have a NULL
parent (except for the root device, but that's a different story...)

thanks,

greg k-h

Re: [PATCH] thunderbolt: handle possible NULL pointer from get_device()

[Dmitry Antipov]

On 6/9/23 10:46, Greg KH wrote:

> Again, how did you test this?

Did you look at the patch header? For that particular case, the static
analysis tool complains that the value returned by get_device() is most
likely should be checked just because it is checked on a lot of other
code paths. Usually it may be a good precaution to handle the very rare
and unexpected errors; again, if you're sure that this is not the case,
just disregard it.

Dmitry

Re: [PATCH] thunderbolt: handle possible NULL pointer from get_device()

[Greg KH]

On Fri, Jun 09, 2023 at 11:05:59AM +0300, Dmitry Antipov wrote:
> On 6/9/23 10:46, Greg KH wrote:
> 
> > Again, how did you test this?
> 
> Did you look at the patch header? For that particular case, the static
> analysis tool complains that the value returned by get_device() is most
> likely should be checked just because it is checked on a lot of other
> code paths. Usually it may be a good precaution to handle the very rare
> and unexpected errors; again, if you're sure that this is not the case,
> just disregard it.

Just because a static tool said "this might be wrong" does not mean you
do not need to actually test your change or do some work to verify that
it is a sane change at all.

So far I have seen more and more false-positives from this "tool" of
your group that I am very inclined to just tell all kernel maintainers
to ignore them for a very long time as you are not following the
documented rules for such patches as outlined in
Documentation/process/researcher-guidelines.rst

Please read that and fix your tool, and your submission process, I've
said this many times already.

greg k-h

Re: [PATCH] thunderbolt: handle possible NULL pointer from get_device()

[Dmitry Antipov]

On 6/9/23 11:20, Greg KH wrote:

> So far I have seen more and more false-positives from this "tool" of
> your group that I am very inclined to just tell all kernel maintainers
> to ignore them for a very long time as you are not following the
> documented rules for such patches as outlined in
> Documentation/process/researcher-guidelines.rst
> 
> Please read that and fix your tool, and your submission process, I've
> said this many times already.

There might be a lot of definitions of what "research" actually is.
I realize that the maintainers may be very busy, but is it completely
illegal to sent a patch just to raise the flag and ask to share an
expertise? Note this was not a private e-mail, and it looks a bit
strange to treat it just like an attempt to waste your personal time.
And don't you think that such a policy definitely cuts off the
beginners and makes the community less friendly?

Dmitry

Re: [PATCH] thunderbolt: handle possible NULL pointer from get_device()

[Greg KH]

On Fri, Jun 09, 2023 at 12:19:02PM +0300, Dmitry Antipov wrote:
> On 6/9/23 11:20, Greg KH wrote:
> 
> > So far I have seen more and more false-positives from this "tool" of
> > your group that I am very inclined to just tell all kernel maintainers
> > to ignore them for a very long time as you are not following the
> > documented rules for such patches as outlined in
> > Documentation/process/researcher-guidelines.rst
> > 
> > Please read that and fix your tool, and your submission process, I've
> > said this many times already.
> 
> There might be a lot of definitions of what "research" actually is.
> I realize that the maintainers may be very busy, but is it completely
> illegal to sent a patch just to raise the flag and ask to share an
> expertise? Note this was not a private e-mail, and it looks a bit
> strange to treat it just like an attempt to waste your personal time.

As a maintainer, you are asking for others to check your work.  If you
are sending stuff that is incorrect, yes, that is wasting their time.

> And don't you think that such a policy definitely cuts off the
> beginners and makes the community less friendly?

Again, you are asking me to verify that your random tool is somehow
actually sending out correct messages by asking me to review a patch for
it?  It's your responsibility to verify that your tool works properly,
as you are in control of it, so yes, you MUST follow those research
guidelines as that is EXACTLY what they were written for due to problems
exactly like this.

I have pointed out how broken this "tool" is numerous times in the past,
this should not come as a surprise to you at all.

greg k-h

Re: [lvc-project] [PATCH] thunderbolt: handle possible NULL pointer from get_device()

[Alexey Khoroshilov]

On 09.06.2023 11:05, Dmitry Antipov wrote:
> On 6/9/23 10:46, Greg KH wrote:
> 
>> Again, how did you test this?
> 
> Did you look at the patch header? For that particular case, the static
> analysis tool complains that the value returned by get_device() is most
> likely should be checked just because it is checked on a lot of other
> code paths. Usually it may be a good precaution to handle the very rare
> and unexpected errors; again, if you're sure that this is not the case,
> just disregard it.

Nevertheless it is your responsibility to check if such situation can
happen in practice in this particular place.

And if there are non-obvious assumptions that prevent you to make such
analysis before sending a patch, you are welcome to prepare a patch that
improves documentation.

Best regards,
Alexey