[UNTESTED PATCH] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet

[UNTESTED PATCH] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet

[Miko Larsson]

From ef617d8df22945b871ab989e25c07d7c60ae21f6 Mon Sep 17 00:00:00 2001
From: Miko Larsson <mikoxyzzz@gmail.com>
Date: Tue, 31 Jan 2023 11:01:20 +0100
Subject: [UNTESTED PATCH] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet

syzbot reports that act_len in kalmia_send_init_packet() is
uninitialized. Attempt to fix this by initializing it to 0.

Fixes: d40261236e8e ("net/usb: Add Samsung Kalmia driver for Samsung GT-B3730")
Reported-by: syzbot+cd80c5ef5121bfe85b55@syzkaller.appspotmail.com
Signed-off-by: Miko Larsson <mikoxyzzz@gmail.com>
---
 drivers/net/usb/kalmia.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c
index 9f2b70ef39aa..b158fb7bf66a 100644
--- a/drivers/net/usb/kalmia.c
+++ b/drivers/net/usb/kalmia.c
@@ -56,7 +56,7 @@ static int
 kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
 	u8 *buffer, u8 expected_len)
 {
-	int act_len;
+	int act_len = 0;
 	int status;
 
 	netdev_dbg(dev->net, "Sending init packet");
-- 
2.39.1

Re: [UNTESTED PATCH] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet

[Greg KH]

On Tue, Jan 31, 2023 at 12:14:54PM +0100, Miko Larsson wrote:
> >From ef617d8df22945b871ab989e25c07d7c60ae21f6 Mon Sep 17 00:00:00 2001
> From: Miko Larsson <mikoxyzzz@gmail.com>
> Date: Tue, 31 Jan 2023 11:01:20 +0100

Why is this in the changelog text?

> Subject: [UNTESTED PATCH] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet
> 
> syzbot reports that act_len in kalmia_send_init_packet() is
> uninitialized. Attempt to fix this by initializing it to 0.

You can send patches to syzbot to have it test things, have you tried
that?

thanks,

greg k-h

Re: [UNTESTED PATCH] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet

[Miko Larsson]

On Tue, 2023-01-31 at 12:37 +0100, Greg KH wrote:
> On Tue, Jan 31, 2023 at 12:14:54PM +0100, Miko Larsson wrote:
> > > From ef617d8df22945b871ab989e25c07d7c60ae21f6 Mon Sep 17 00:00:00
> > > 2001
> > From: Miko Larsson <mikoxyzzz@gmail.com>
> > Date: Tue, 31 Jan 2023 11:01:20 +0100
> 
> Why is this in the changelog text?
D'oh, sorry about that. Imported the patch directly into Evolution
without trimming that away.

> > Subject: [UNTESTED PATCH] net/usb: kalmia: Fix uninit-value in
> > kalmia_send_init_packet
> > 
> > syzbot reports that act_len in kalmia_send_init_packet() is
> > uninitialized. Attempt to fix this by initializing it to 0.
> 
> You can send patches to syzbot to have it test things, have you tried
> that?
Didn't know that, will try!

-- 
~miko

[PATCH v2] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet

[Miko Larsson]

syzbot reports that act_len in kalmia_send_init_packet() is
uninitialized. Fix this by initializing it to 0.

Fixes: d40261236e8e ("net/usb: Add Samsung Kalmia driver for Samsung GT-B3730")
Reported-and-tested-by: syzbot+cd80c5ef5121bfe85b55@syzkaller.appspotmail.com
Signed-off-by: Miko Larsson <mikoxyzzz@gmail.com>
---
v1 -> v2
* Minor alteration of commit message.
* Added 'reported-and-tested-by' which is attributed to syzbot.

 drivers/net/usb/kalmia.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c
index 9f2b70ef39aa..b158fb7bf66a 100644
--- a/drivers/net/usb/kalmia.c
+++ b/drivers/net/usb/kalmia.c
@@ -56,7 +56,7 @@ static int
 kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
 	u8 *buffer, u8 expected_len)
 {
-	int act_len;
+	int act_len = 0;
 	int status;
 
 	netdev_dbg(dev->net, "Sending init packet");
-- 
2.39.1

Re: [PATCH v2] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet

[Greg KH]

On Tue, Jan 31, 2023 at 03:20:33PM +0100, Miko Larsson wrote:
> syzbot reports that act_len in kalmia_send_init_packet() is
> uninitialized. Fix this by initializing it to 0.
> 
> Fixes: d40261236e8e ("net/usb: Add Samsung Kalmia driver for Samsung GT-B3730")
> Reported-and-tested-by: syzbot+cd80c5ef5121bfe85b55@syzkaller.appspotmail.com
> Signed-off-by: Miko Larsson <mikoxyzzz@gmail.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v2] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet

[Jiri Pirko]

Tue, Jan 31, 2023 at 03:20:33PM CET, mikoxyzzz@gmail.com wrote:
>syzbot reports that act_len in kalmia_send_init_packet() is
>uninitialized. Fix this by initializing it to 0.
>
>Fixes: d40261236e8e ("net/usb: Add Samsung Kalmia driver for Samsung GT-B3730")
>Reported-and-tested-by: syzbot+cd80c5ef5121bfe85b55@syzkaller.appspotmail.com
>Signed-off-by: Miko Larsson <mikoxyzzz@gmail.com>
>---
>v1 -> v2
>* Minor alteration of commit message.
>* Added 'reported-and-tested-by' which is attributed to syzbot.
>
> drivers/net/usb/kalmia.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c
>index 9f2b70ef39aa..b158fb7bf66a 100644
>--- a/drivers/net/usb/kalmia.c
>+++ b/drivers/net/usb/kalmia.c
>@@ -56,7 +56,7 @@ static int
> kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
> 	u8 *buffer, u8 expected_len)
> {
>-	int act_len;
>+	int act_len = 0;
> 	int status;
> 
> 	netdev_dbg(dev->net, "Sending init packet");

Hmm, this is not the right fix.

If the second call of usb_bulk_msg() in this function returns != 0, the
act_len printed out contains the value from previous usb_bulk_msg() call,
which does not make sense.

Printing act_len on error path is pointless, so rather remove it from
the error message entirely for both usb_bulk_msg() calls.

Re: [PATCH v2] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet

[Miko Larsson]

On Wed, 2023-02-01 at 13:19 +0100, Jiri Pirko wrote:
> Tue, Jan 31, 2023 at 03:20:33PM CET, mikoxyzzz@gmail.com wrote:
> > syzbot reports that act_len in kalmia_send_init_packet() is
> > uninitialized. Fix this by initializing it to 0.
> > 
> > Fixes: d40261236e8e ("net/usb: Add Samsung Kalmia driver for
> > Samsung GT-B3730")
> > Reported-and-tested-by:
> > syzbot+cd80c5ef5121bfe85b55@syzkaller.appspotmail.com
> > Signed-off-by: Miko Larsson <mikoxyzzz@gmail.com>
> > ---
> > v1 -> v2
> > * Minor alteration of commit message.
> > * Added 'reported-and-tested-by' which is attributed to syzbot.
> > 
> > drivers/net/usb/kalmia.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c
> > index 9f2b70ef39aa..b158fb7bf66a 100644
> > --- a/drivers/net/usb/kalmia.c
> > +++ b/drivers/net/usb/kalmia.c
> > @@ -56,7 +56,7 @@ static int
> > kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8
> > init_msg_len,
> >         u8 *buffer, u8 expected_len)
> > {
> > -       int act_len;
> > +       int act_len = 0;
> >         int status;
> > 
> >         netdev_dbg(dev->net, "Sending init packet");
> 
> Hmm, this is not the right fix.
> 
> If the second call of usb_bulk_msg() in this function returns != 0,
> the
> act_len printed out contains the value from previous usb_bulk_msg()
> call,
> which does not make sense.
> 
> Printing act_len on error path is pointless, so rather remove it from
> the error message entirely for both usb_bulk_msg() calls.

Something like this, then?

diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c
index 9f2b70ef39aa..613fc6910f14 100644
--- a/drivers/net/usb/kalmia.c
+++ b/drivers/net/usb/kalmia.c
@@ -65,8 +65,8 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
 		init_msg, init_msg_len, &act_len, KALMIA_USB_TIMEOUT);
 	if (status != 0) {
 		netdev_err(dev->net,
-			"Error sending init packet. Status %i, length %i\n",
-			status, act_len);
+			"Error sending init packet. Status %i\n",
+			status);
 		return status;
 	}
 	else if (act_len != init_msg_len) {
@@ -83,8 +83,8 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
 
 	if (status != 0)
 		netdev_err(dev->net,
-			"Error receiving init result. Status %i, length %i\n",
-			status, act_len);
+			"Error receiving init result. Status %i\n",
+			status);
 	else if (act_len != expected_len)
 		netdev_err(dev->net, "Unexpected init result length: %i\n",
 			act_len);

-- 
~miko

Re: [PATCH v2] net/usb: kalmia: Fix uninit-value in kalmia_send_init_packet

[Jiri Pirko]

Thu, Feb 09, 2023 at 03:47:12PM CET, mikoxyzzz@gmail.com wrote:
>On Wed, 2023-02-01 at 13:19 +0100, Jiri Pirko wrote:
>> Tue, Jan 31, 2023 at 03:20:33PM CET, mikoxyzzz@gmail.com wrote:
>> > syzbot reports that act_len in kalmia_send_init_packet() is
>> > uninitialized. Fix this by initializing it to 0.
>> > 
>> > Fixes: d40261236e8e ("net/usb: Add Samsung Kalmia driver for
>> > Samsung GT-B3730")
>> > Reported-and-tested-by:
>> > syzbot+cd80c5ef5121bfe85b55@syzkaller.appspotmail.com
>> > Signed-off-by: Miko Larsson <mikoxyzzz@gmail.com>
>> > ---
>> > v1 -> v2
>> > * Minor alteration of commit message.
>> > * Added 'reported-and-tested-by' which is attributed to syzbot.
>> > 
>> > drivers/net/usb/kalmia.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c
>> > index 9f2b70ef39aa..b158fb7bf66a 100644
>> > --- a/drivers/net/usb/kalmia.c
>> > +++ b/drivers/net/usb/kalmia.c
>> > @@ -56,7 +56,7 @@ static int
>> > kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8
>> > init_msg_len,
>> >         u8 *buffer, u8 expected_len)
>> > {
>> > -       int act_len;
>> > +       int act_len = 0;
>> >         int status;
>> > 
>> >         netdev_dbg(dev->net, "Sending init packet");
>> 
>> Hmm, this is not the right fix.
>> 
>> If the second call of usb_bulk_msg() in this function returns != 0,
>> the
>> act_len printed out contains the value from previous usb_bulk_msg()
>> call,
>> which does not make sense.
>> 
>> Printing act_len on error path is pointless, so rather remove it from
>> the error message entirely for both usb_bulk_msg() calls.
>
>Something like this, then?

Yes.

>
>diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c
>index 9f2b70ef39aa..613fc6910f14 100644
>--- a/drivers/net/usb/kalmia.c
>+++ b/drivers/net/usb/kalmia.c
>@@ -65,8 +65,8 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
> 		init_msg, init_msg_len, &act_len, KALMIA_USB_TIMEOUT);
> 	if (status != 0) {
> 		netdev_err(dev->net,
>-			"Error sending init packet. Status %i, length %i\n",
>-			status, act_len);
>+			"Error sending init packet. Status %i\n",
>+			status);
> 		return status;
> 	}
> 	else if (act_len != init_msg_len) {
>@@ -83,8 +83,8 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
> 
> 	if (status != 0)
> 		netdev_err(dev->net,
>-			"Error receiving init result. Status %i, length %i\n",
>-			status, act_len);
>+			"Error receiving init result. Status %i\n",
>+			status);
> 	else if (act_len != expected_len)
> 		netdev_err(dev->net, "Unexpected init result length: %i\n",
> 			act_len);
>
>-- 
>~miko

[PATCH] net/usb: kalmia: Don't pass act_len in usb_bulk_msg error path

[Miko Larsson]

syzbot reported that act_len in kalmia_send_init_packet() is
uninitialized when passing it to the first usb_bulk_msg error path. Jiri
Pirko noted that it's pointless to pass it in the error path, and that
the value that would be printed in the second error path would be the
value of act_len from the first call to usb_bulk_msg.[1]

With this in mind, let's just not pass act_len to the usb_bulk_msg error
paths.

1: https://lore.kernel.org/lkml/Y9pY61y1nwTuzMOa@nanopsycho/

Fixes: d40261236e8e ("net/usb: Add Samsung Kalmia driver for Samsung GT-B3730")
Reported-and-tested-by: syzbot+cd80c5ef5121bfe85b55@syzkaller.appspotmail.com
Signed-off-by: Miko Larsson <mikoxyzzz@gmail.com>
---
 drivers/net/usb/kalmia.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c
index 9f2b70ef39aa..613fc6910f14 100644
--- a/drivers/net/usb/kalmia.c
+++ b/drivers/net/usb/kalmia.c
@@ -65,8 +65,8 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
 		init_msg, init_msg_len, &act_len, KALMIA_USB_TIMEOUT);
 	if (status != 0) {
 		netdev_err(dev->net,
-			"Error sending init packet. Status %i, length %i\n",
-			status, act_len);
+			"Error sending init packet. Status %i\n",
+			status);
 		return status;
 	}
 	else if (act_len != init_msg_len) {
@@ -83,8 +83,8 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
 
 	if (status != 0)
 		netdev_err(dev->net,
-			"Error receiving init result. Status %i, length %i\n",
-			status, act_len);
+			"Error receiving init result. Status %i\n",
+			status);
 	else if (act_len != expected_len)
 		netdev_err(dev->net, "Unexpected init result length: %i\n",
 			act_len);
-- 
2.39.1

Re: [PATCH] net/usb: kalmia: Don't pass act_len in usb_bulk_msg error path

[Alexander H Duyck]

On Fri, 2023-02-10 at 09:13 +0100, Miko Larsson wrote:
> syzbot reported that act_len in kalmia_send_init_packet() is
> uninitialized when passing it to the first usb_bulk_msg error path. Jiri
> Pirko noted that it's pointless to pass it in the error path, and that
> the value that would be printed in the second error path would be the
> value of act_len from the first call to usb_bulk_msg.[1]
> 
> With this in mind, let's just not pass act_len to the usb_bulk_msg error
> paths.
> 
> 1: https://lore.kernel.org/lkml/Y9pY61y1nwTuzMOa@nanopsycho/
> 
> Fixes: d40261236e8e ("net/usb: Add Samsung Kalmia driver for Samsung GT-B3730")
> Reported-and-tested-by: syzbot+cd80c5ef5121bfe85b55@syzkaller.appspotmail.com
> Signed-off-by: Miko Larsson <mikoxyzzz@gmail.com>
> ---
>  drivers/net/usb/kalmia.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c
> index 9f2b70ef39aa..613fc6910f14 100644
> --- a/drivers/net/usb/kalmia.c
> +++ b/drivers/net/usb/kalmia.c
> @@ -65,8 +65,8 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
>  		init_msg, init_msg_len, &act_len, KALMIA_USB_TIMEOUT);
>  	if (status != 0) {
>  		netdev_err(dev->net,
> -			"Error sending init packet. Status %i, length %i\n",
> -			status, act_len);
> +			"Error sending init packet. Status %i\n",
> +			status);
>  		return status;
>  	}
>  	else if (act_len != init_msg_len) {
> @@ -83,8 +83,8 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len,
>  
>  	if (status != 0)
>  		netdev_err(dev->net,
> -			"Error receiving init result. Status %i, length %i\n",
> -			status, act_len);
> +			"Error receiving init result. Status %i\n",
> +			status);
>  	else if (act_len != expected_len)
>  		netdev_err(dev->net, "Unexpected init result length: %i\n",
>  			act_len);

Makes sense to me since the only possible return values for act_len
appear to be either uninitialized or 0 depending on where it fails.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Re: [PATCH] net/usb: kalmia: Don't pass act_len in usb_bulk_msg error path

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 10 Feb 2023 09:13:44 +0100 you wrote:
> syzbot reported that act_len in kalmia_send_init_packet() is
> uninitialized when passing it to the first usb_bulk_msg error path. Jiri
> Pirko noted that it's pointless to pass it in the error path, and that
> the value that would be printed in the second error path would be the
> value of act_len from the first call to usb_bulk_msg.[1]
> 
> With this in mind, let's just not pass act_len to the usb_bulk_msg error
> paths.
> 
> [...]

Here is the summary with links:
  - net/usb: kalmia: Don't pass act_len in usb_bulk_msg error path
    https://git.kernel.org/netdev/net/c/c68f345b7c42

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html