linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] [PATCH v2 net] rose: Fix Null pointer dereference in rose_send_frame()
@ 2020-11-05 15:56 Anmol Karn
  2020-11-06 21:04 ` Saeed Mahameed
  2020-11-07 18:56 ` [Linux-kernel-mentees] [PATCH v2] net: " Anmol Karn
  0 siblings, 2 replies; 13+ messages in thread
From: Anmol Karn @ 2020-11-05 15:56 UTC (permalink / raw)
  To: ralf, davem, kuba
  Cc: gregkh, netdev, linux-kernel, linux-hams, linux-kernel-mentees,
	syzkaller-bugs, anmol.karan123, syzbot+a1c743815982d9496393

rose_send_frame() dereferences `neigh->dev` when called from
rose_transmit_clear_request(), and the first occurance of the `neigh`
is in rose_loopback_timer() as `rose_loopback_neigh`, and it is initialized
in rose_add_loopback_neigh() as NULL. i.e when `rose_loopback_neigh` used in 
rose_loopback_timer() its `->dev` was still NULL and rose_loopback_timer() 
was calling rose_rx_call_request() without checking for NULL.

- net/rose/rose_link.c
This bug seems to get triggered in this line:

rose_call = (ax25_address *)neigh->dev->dev_addr;

Fix it by adding NULL checking for `rose_loopback_neigh->dev` in rose_loopback_timer(). 

Reported-and-tested-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com 
Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3 
Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>
---
 net/rose/rose_loopback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index 7b094275ea8b..cd7774cb1d07 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -96,7 +96,7 @@ static void rose_loopback_timer(struct timer_list *unused)
 		}
 
 		if (frametype == ROSE_CALL_REQUEST) {
-			if ((dev = rose_dev_get(dest)) != NULL) {
+			if (rose_loopback_neigh->dev && (dev = rose_dev_get(dest)) != NULL) {
 				if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
 					kfree_skb(skb);
 			} else {
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 net] rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-05 15:56 [Linux-kernel-mentees] [PATCH v2 net] rose: Fix Null pointer dereference in rose_send_frame() Anmol Karn
@ 2020-11-06 21:04 ` Saeed Mahameed
  2020-11-07  8:20   ` Anmol Karn
  2020-11-07 18:56 ` [Linux-kernel-mentees] [PATCH v2] net: " Anmol Karn
  1 sibling, 1 reply; 13+ messages in thread
From: Saeed Mahameed @ 2020-11-06 21:04 UTC (permalink / raw)
  To: Anmol Karn, ralf, davem, kuba
  Cc: gregkh, netdev, linux-kernel, linux-hams, linux-kernel-mentees,
	syzkaller-bugs, syzbot+a1c743815982d9496393

On Thu, 2020-11-05 at 21:26 +0530, Anmol Karn wrote:
> rose_send_frame() dereferences `neigh->dev` when called from
> rose_transmit_clear_request(), and the first occurance of the `neigh`
> is in rose_loopback_timer() as `rose_loopback_neigh`, and it is
> initialized
> in rose_add_loopback_neigh() as NULL. i.e when `rose_loopback_neigh`
> used in 
> rose_loopback_timer() its `->dev` was still NULL and
> rose_loopback_timer() 
> was calling rose_rx_call_request() without checking for NULL.
> 
> - net/rose/rose_link.c
> This bug seems to get triggered in this line:
> 
> rose_call = (ax25_address *)neigh->dev->dev_addr;
> 
> Fix it by adding NULL checking for `rose_loopback_neigh->dev` in
> rose_loopback_timer(). 
> 
> Reported-and-tested-by: 
> syzbot+a1c743815982d9496393@syzkaller.appspotmail.com 
> Link: 
> https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
>  
> Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>

missing proper fixes tag.

> ---
>  net/rose/rose_loopback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
> index 7b094275ea8b..cd7774cb1d07 100644
> --- a/net/rose/rose_loopback.c
> +++ b/net/rose/rose_loopback.c
> @@ -96,7 +96,7 @@ static void rose_loopback_timer(struct timer_list
> *unused)
>  		}
>  
>  		if (frametype == ROSE_CALL_REQUEST) {
> -			if ((dev = rose_dev_get(dest)) != NULL) {
> +			if (rose_loopback_neigh->dev && (dev =
> rose_dev_get(dest)) != NULL) {
>  				if (rose_rx_call_request(skb, dev,
> rose_loopback_neigh, lci_o) == 0)
>  					kfree_skb(skb);
>  			} else {

check patch is not happy:

WARNING:TYPO_SPELLING: 'occurance' may be misspelled - perhaps
'occurrence'?
#7: 
rose_transmit_clear_request(), and the first occurance of the `neigh`

ERROR:ASSIGN_IN_IF: do not use assignment in if condition
#36: FILE: net/rose/rose_loopback.c:99:
+                       if (rose_loopback_neigh->dev && (dev =
rose_dev_get(dest)) != NULL) {

total: 1 errors, 1 warnings, 0 checks, 8 lines checked



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 net] rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-06 21:04 ` Saeed Mahameed
@ 2020-11-07  8:20   ` Anmol Karn
  2020-11-07 19:18     ` [Linux-kernel-mentees] [PATCH v3 " Anmol Karn
  0 siblings, 1 reply; 13+ messages in thread
From: Anmol Karn @ 2020-11-07  8:20 UTC (permalink / raw)
  To: Saeed Mahameed
  Cc: ralf, davem, kuba, gregkh, netdev, linux-kernel, linux-hams,
	linux-kernel-mentees, syzkaller-bugs,
	syzbot+a1c743815982d9496393

Hello Sir,

On Fri, Nov 06, 2020 at 01:04:27PM -0800, Saeed Mahameed wrote:
> On Thu, 2020-11-05 at 21:26 +0530, Anmol Karn wrote:
> > rose_send_frame() dereferences `neigh->dev` when called from
> > rose_transmit_clear_request(), and the first occurance of the `neigh`
> > is in rose_loopback_timer() as `rose_loopback_neigh`, and it is
> > initialized
> > in rose_add_loopback_neigh() as NULL. i.e when `rose_loopback_neigh`
> > used in 
> > rose_loopback_timer() its `->dev` was still NULL and
> > rose_loopback_timer() 
> > was calling rose_rx_call_request() without checking for NULL.
> > 
> > - net/rose/rose_link.c
> > This bug seems to get triggered in this line:
> > 
> > rose_call = (ax25_address *)neigh->dev->dev_addr;
> > 
> > Fix it by adding NULL checking for `rose_loopback_neigh->dev` in
> > rose_loopback_timer(). 
> > 
> > Reported-and-tested-by: 
> > syzbot+a1c743815982d9496393@syzkaller.appspotmail.com 
> > Link: 
> > https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
> >  
> > Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>
> 
> missing proper fixes tag.
> 
> > ---
> >  net/rose/rose_loopback.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
> > index 7b094275ea8b..cd7774cb1d07 100644
> > --- a/net/rose/rose_loopback.c
> > +++ b/net/rose/rose_loopback.c
> > @@ -96,7 +96,7 @@ static void rose_loopback_timer(struct timer_list
> > *unused)
> >  		}
> >  
> >  		if (frametype == ROSE_CALL_REQUEST) {
> > -			if ((dev = rose_dev_get(dest)) != NULL) {
> > +			if (rose_loopback_neigh->dev && (dev =
> > rose_dev_get(dest)) != NULL) {
> >  				if (rose_rx_call_request(skb, dev,
> > rose_loopback_neigh, lci_o) == 0)
> >  					kfree_skb(skb);
> >  			} else {
> 
> check patch is not happy:
> 
> WARNING:TYPO_SPELLING: 'occurance' may be misspelled - perhaps
> 'occurrence'?
> #7: 
> rose_transmit_clear_request(), and the first occurance of the `neigh`
> 
> ERROR:ASSIGN_IN_IF: do not use assignment in if condition
> #36: FILE: net/rose/rose_loopback.c:99:
> +                       if (rose_loopback_neigh->dev && (dev =
> rose_dev_get(dest)) != NULL) {
> 
> total: 1 errors, 1 warnings, 0 checks, 8 lines checked
> 
> 

Thank you for your review will rectify these and send another version.

Thanks,
Anmol

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Linux-kernel-mentees] [PATCH v2] net: rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-05 15:56 [Linux-kernel-mentees] [PATCH v2 net] rose: Fix Null pointer dereference in rose_send_frame() Anmol Karn
  2020-11-06 21:04 ` Saeed Mahameed
@ 2020-11-07 18:56 ` Anmol Karn
  2020-11-07 19:02   ` Anmol karn
  1 sibling, 1 reply; 13+ messages in thread
From: Anmol Karn @ 2020-11-07 18:56 UTC (permalink / raw)
  To: ralf, davem, kuba
  Cc: saeed, gregkh, netdev, linux-kernel, linux-hams,
	linux-kernel-mentees, syzkaller-bugs, anmol.karan123,
	syzbot+a1c743815982d9496393

rose_send_frame() dereferences `neigh->dev` when called from
rose_transmit_clear_request(), and the first occurrence of the
`neigh` is in rose_loopback_timer() as `rose_loopback_neigh`,
and it is initialized in rose_add_loopback_neigh() as NULL.
i.e when `rose_loopback_neigh` used in rose_loopback_timer()
its `->dev` was still NULL and rose_loopback_timer() was calling
rose_rx_call_request() without checking for NULL.

- net/rose/rose_link.c
This bug seems to get triggered in this line:

rose_call = (ax25_address *)neigh->dev->dev_addr;

Fix it by adding NULL checking for `rose_loopback_neigh->dev`
in rose_loopback_timer().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
Tested-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>
---
Changes in v3:
        - Corrected checkpatch warnings and errors (Suggested-by: Saeed Mahameed <saeed@kernel.org>)
	- Added "Fixes:" tag (Suggested-by: Saeed Mahameed <saeed@kernel.org>)
Changes in v2:
	- Added NULL check in rose_loopback_timer() (Suggested-by: Greg KH <gregkh@linuxfoundation.org>)

 net/rose/rose_loopback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index 7b094275ea8b..2c51756ed7bf 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -96,7 +96,8 @@ static void rose_loopback_timer(struct timer_list *unused)
 		}

 		if (frametype == ROSE_CALL_REQUEST) {
-			if ((dev = rose_dev_get(dest)) != NULL) {
+			dev = rose_dev_get(dest);
+			if (rose_loopback_neigh->dev && dev) {
 				if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
 					kfree_skb(skb);
 			} else {
-
2.29.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] net: rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-07 18:56 ` [Linux-kernel-mentees] [PATCH v2] net: " Anmol Karn
@ 2020-11-07 19:02   ` Anmol karn
  0 siblings, 0 replies; 13+ messages in thread
From: Anmol karn @ 2020-11-07 19:02 UTC (permalink / raw)
  To: ralf, David S. Miller, Jakub Kicinski
  Cc: Saeed Mahameed, Greg KH, netdev, open list, linux-hams,
	linux-kernel-mentees, syzkaller-bugs, syzbot

Hello,

Sorry for this wrong subject(It should be v3 instead of v2),
please ignore this patch.

I will send a new one with the corrected subject.

Thanks,
Anmol

On Sun, Nov 8, 2020 at 12:27 AM Anmol Karn <anmol.karan123@gmail.com> wrote:
>
> rose_send_frame() dereferences `neigh->dev` when called from
> rose_transmit_clear_request(), and the first occurrence of the
> `neigh` is in rose_loopback_timer() as `rose_loopback_neigh`,
> and it is initialized in rose_add_loopback_neigh() as NULL.
> i.e when `rose_loopback_neigh` used in rose_loopback_timer()
> its `->dev` was still NULL and rose_loopback_timer() was calling
> rose_rx_call_request() without checking for NULL.
>
> - net/rose/rose_link.c
> This bug seems to get triggered in this line:
>
> rose_call = (ax25_address *)neigh->dev->dev_addr;
>
> Fix it by adding NULL checking for `rose_loopback_neigh->dev`
> in rose_loopback_timer().
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
> Tested-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
> Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>
> ---
> Changes in v3:
>         - Corrected checkpatch warnings and errors (Suggested-by: Saeed Mahameed <saeed@kernel.org>)
>         - Added "Fixes:" tag (Suggested-by: Saeed Mahameed <saeed@kernel.org>)
> Changes in v2:
>         - Added NULL check in rose_loopback_timer() (Suggested-by: Greg KH <gregkh@linuxfoundation.org>)
>
>  net/rose/rose_loopback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
> index 7b094275ea8b..2c51756ed7bf 100644
> --- a/net/rose/rose_loopback.c
> +++ b/net/rose/rose_loopback.c
> @@ -96,7 +96,8 @@ static void rose_loopback_timer(struct timer_list *unused)
>                 }
>
>                 if (frametype == ROSE_CALL_REQUEST) {
> -                       if ((dev = rose_dev_get(dest)) != NULL) {
> +                       dev = rose_dev_get(dest);
> +                       if (rose_loopback_neigh->dev && dev) {
>                                 if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
>                                         kfree_skb(skb);
>                         } else {
> -
> 2.29.2
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Linux-kernel-mentees] [PATCH v3 net] rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-07  8:20   ` Anmol Karn
@ 2020-11-07 19:18     ` Anmol Karn
  2020-11-10 17:58       ` Jakub Kicinski
  0 siblings, 1 reply; 13+ messages in thread
From: Anmol Karn @ 2020-11-07 19:18 UTC (permalink / raw)
  To: ralf, davem, kuba
  Cc: saeed, gregkh, netdev, linux-kernel, linux-hams,
	linux-kernel-mentees, syzkaller-bugs, anmol.karan123,
	syzbot+a1c743815982d9496393

rose_send_frame() dereferences `neigh->dev` when called from
rose_transmit_clear_request(), and the first occurrence of the
`neigh` is in rose_loopback_timer() as `rose_loopback_neigh`,
and it is initialized in rose_add_loopback_neigh() as NULL.
i.e when `rose_loopback_neigh` used in rose_loopback_timer()
its `->dev` was still NULL and rose_loopback_timer() was calling
rose_rx_call_request() without checking for NULL.

- net/rose/rose_link.c
This bug seems to get triggered in this line:

rose_call = (ax25_address *)neigh->dev->dev_addr;

Fix it by adding NULL checking for `rose_loopback_neigh->dev`
in rose_loopback_timer().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
Tested-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>
---
Changes in v3:
        - Corrected checkpatch warnings and errors (Suggested-by: Saeed Mahameed <saeed@kernel.org>)
	- Added "Fixes:" tag (Suggested-by: Saeed Mahameed <saeed@kernel.org>)
Changes in v2:
	- Added NULL check in rose_loopback_timer() (Suggested-by: Greg KH <gregkh@linuxfoundation.org>)

 net/rose/rose_loopback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index 7b094275ea8b..2c51756ed7bf 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -96,7 +96,8 @@ static void rose_loopback_timer(struct timer_list *unused)
 		}

 		if (frametype == ROSE_CALL_REQUEST) {
-			if ((dev = rose_dev_get(dest)) != NULL) {
+			dev = rose_dev_get(dest);
+			if (rose_loopback_neigh->dev && dev) {
 				if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
 					kfree_skb(skb);
 			} else {
-
2.29.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3 net] rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-07 19:18     ` [Linux-kernel-mentees] [PATCH v3 " Anmol Karn
@ 2020-11-10 17:58       ` Jakub Kicinski
  2020-11-10 19:45         ` Anmol Karn
  0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2020-11-10 17:58 UTC (permalink / raw)
  To: Anmol Karn
  Cc: ralf, davem, saeed, gregkh, netdev, linux-kernel, linux-hams,
	linux-kernel-mentees, syzkaller-bugs,
	syzbot+a1c743815982d9496393

On Sun,  8 Nov 2020 00:48:35 +0530 Anmol Karn wrote:
> +			dev = rose_dev_get(dest);

this calls dev_hold internally, you never release that reference in
case ..neigh->dev is NULL

> +			if (rose_loopback_neigh->dev && dev) {

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3 net] rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-10 17:58       ` Jakub Kicinski
@ 2020-11-10 19:45         ` Anmol Karn
  2020-11-11 16:59           ` [Linux-kernel-mentees] [PATCH v4 " Anmol Karn
  0 siblings, 1 reply; 13+ messages in thread
From: Anmol Karn @ 2020-11-10 19:45 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: ralf, davem, saeed, gregkh, netdev, linux-kernel, linux-hams,
	linux-kernel-mentees, syzkaller-bugs,
	syzbot+a1c743815982d9496393

Hello Sir,

On Tue, Nov 10, 2020 at 09:58:15AM -0800, Jakub Kicinski wrote:
> On Sun,  8 Nov 2020 00:48:35 +0530 Anmol Karn wrote:
> > +			dev = rose_dev_get(dest);
> 
> this calls dev_hold internally, you never release that reference in
> case ..neigh->dev is NULL
> 
> > +			if (rose_loopback_neigh->dev && dev) {

Ah, I missed to `dev_put()` the `dev` after checking for, if neigh->dev is NULL,
I will fix it soon and send another version.

Thank you for review.


Anmol

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Linux-kernel-mentees] [PATCH v4 net] rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-10 19:45         ` Anmol Karn
@ 2020-11-11 16:59           ` Anmol Karn
  2020-11-14 19:18             ` Jakub Kicinski
  0 siblings, 1 reply; 13+ messages in thread
From: Anmol Karn @ 2020-11-11 16:59 UTC (permalink / raw)
  To: ralf, davem, kuba
  Cc: saeed, gregkh, netdev, linux-kernel, linux-hams,
	linux-kernel-mentees, syzkaller-bugs, anmol.karan123,
	syzbot+a1c743815982d9496393

rose_send_frame() dereferences `neigh->dev` when called from
rose_transmit_clear_request(), and the first occurrence of the
`neigh` is in rose_loopback_timer() as `rose_loopback_neigh`,
and it is initialized in rose_add_loopback_neigh() as NULL.
i.e when `rose_loopback_neigh` used in rose_loopback_timer()
its `->dev` was still NULL and rose_loopback_timer() was calling
rose_rx_call_request() without checking for NULL.

- net/rose/rose_link.c
This bug seems to get triggered in this line:

rose_call = (ax25_address *)neigh->dev->dev_addr;

Fix it by adding NULL checking for `rose_loopback_neigh->dev`
in rose_loopback_timer().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
Tested-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>
---
Changes in v4:
	- Free `dev`(on dev_hold()), when neigh->dev is NULL. (Suggested-by: Jakub Kicinski <kuba@kernel.org>)
Changes in v3:
        - Corrected checkpatch warnings and errors (Suggested-by: Saeed Mahameed <saeed@kernel.org>)
        - Added "Fixes:" tag (Suggested-by: Saeed Mahameed <saeed@kernel.org>)
Changes in v2:
        - Added NULL check in rose_loopback_timer() (Suggested-by: Greg KH <gregkh@linuxfoundation.org>)

 net/rose/rose_loopback.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index 7b094275ea8b..6a71b6947d92 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -96,10 +96,12 @@ static void rose_loopback_timer(struct timer_list *unused)
 		}

 		if (frametype == ROSE_CALL_REQUEST) {
-			if ((dev = rose_dev_get(dest)) != NULL) {
+			dev = rose_dev_get(dest);
+			if (rose_loopback_neigh->dev && dev) {
 				if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
 					kfree_skb(skb);
 			} else {
+				dev_put(dev);
 				kfree_skb(skb);
 			}
 		} else {
--
2.29.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v4 net] rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-11 16:59           ` [Linux-kernel-mentees] [PATCH v4 " Anmol Karn
@ 2020-11-14 19:18             ` Jakub Kicinski
  2020-11-15 11:44               ` Anmol Karn
  0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2020-11-14 19:18 UTC (permalink / raw)
  To: Anmol Karn
  Cc: ralf, davem, saeed, gregkh, netdev, linux-kernel, linux-hams,
	linux-kernel-mentees, syzkaller-bugs,
	syzbot+a1c743815982d9496393

On Wed, 11 Nov 2020 22:29:54 +0530 Anmol Karn wrote:
> rose_send_frame() dereferences `neigh->dev` when called from
> rose_transmit_clear_request(), and the first occurrence of the
> `neigh` is in rose_loopback_timer() as `rose_loopback_neigh`,
> and it is initialized in rose_add_loopback_neigh() as NULL.
> i.e when `rose_loopback_neigh` used in rose_loopback_timer()
> its `->dev` was still NULL and rose_loopback_timer() was calling
> rose_rx_call_request() without checking for NULL.
> 
> - net/rose/rose_link.c
> This bug seems to get triggered in this line:
> 
> rose_call = (ax25_address *)neigh->dev->dev_addr;
> 
> Fix it by adding NULL checking for `rose_loopback_neigh->dev`
> in rose_loopback_timer().
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
> Tested-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
> Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>

> diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
> index 7b094275ea8b..6a71b6947d92 100644
> --- a/net/rose/rose_loopback.c
> +++ b/net/rose/rose_loopback.c
> @@ -96,10 +96,12 @@ static void rose_loopback_timer(struct timer_list *unused)
>  		}
> 
>  		if (frametype == ROSE_CALL_REQUEST) {
> -			if ((dev = rose_dev_get(dest)) != NULL) {
> +			dev = rose_dev_get(dest);
> +			if (rose_loopback_neigh->dev && dev) {
>  				if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
>  					kfree_skb(skb);
>  			} else {
> +				dev_put(dev);
>  				kfree_skb(skb);
>  			}
>  		} else {

This is still not correct. With this code dev_put() could be called with
NULL, which would cause a crash.

There is also a dev_put() missing if rose_rx_call_request() returns 0.

I think that this is the correct code:

diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index 7b094275ea8b..ff252ef73592 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -96,11 +96,22 @@ static void rose_loopback_timer(struct timer_list *unused)
 		}
 
 		if (frametype == ROSE_CALL_REQUEST) {
-			if ((dev = rose_dev_get(dest)) != NULL) {
-				if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
-					kfree_skb(skb);
-			} else {
+			if (!rose_loopback_neigh->dev) {
 				kfree_skb(skb);
+				continue;
+			}
+
+			dev = rose_dev_get(dest);
+			if (!dev) {
+				kfree_skb(skb);
+				continue;
+			}
+
+			if (rose_rx_call_request(skb, dev, rose_loopback_neigh,
+						 lci_o) == 0) {
+				dev_put(dev);
+				kfree_skb(skb);
 			}
 		} else {
 			kfree_skb(skb);

Please test this and resubmit it if it works.

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v4 net] rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-14 19:18             ` Jakub Kicinski
@ 2020-11-15 11:44               ` Anmol Karn
  2020-11-19 19:10                 ` [Linux-kernel-mentees] [PATCH v5 " Anmol Karn
  0 siblings, 1 reply; 13+ messages in thread
From: Anmol Karn @ 2020-11-15 11:44 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: ralf, davem, saeed, gregkh, netdev, linux-kernel, linux-hams,
	linux-kernel-mentees, syzkaller-bugs,
	syzbot+a1c743815982d9496393

On Sat, Nov 14, 2020 at 11:18:38AM -0800, Jakub Kicinski wrote:
> On Wed, 11 Nov 2020 22:29:54 +0530 Anmol Karn wrote:
> > rose_send_frame() dereferences `neigh->dev` when called from
> > rose_transmit_clear_request(), and the first occurrence of the
> > `neigh` is in rose_loopback_timer() as `rose_loopback_neigh`,
> > and it is initialized in rose_add_loopback_neigh() as NULL.
> > i.e when `rose_loopback_neigh` used in rose_loopback_timer()
> > its `->dev` was still NULL and rose_loopback_timer() was calling
> > rose_rx_call_request() without checking for NULL.
> > 
> > - net/rose/rose_link.c
> > This bug seems to get triggered in this line:
> > 
> > rose_call = (ax25_address *)neigh->dev->dev_addr;
> > 
> > Fix it by adding NULL checking for `rose_loopback_neigh->dev`
> > in rose_loopback_timer().
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Reported-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
> > Tested-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
> > Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>
> 
> > diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
> > index 7b094275ea8b..6a71b6947d92 100644
> > --- a/net/rose/rose_loopback.c
> > +++ b/net/rose/rose_loopback.c
> > @@ -96,10 +96,12 @@ static void rose_loopback_timer(struct timer_list *unused)
> >  		}
> > 
> >  		if (frametype == ROSE_CALL_REQUEST) {
> > -			if ((dev = rose_dev_get(dest)) != NULL) {
> > +			dev = rose_dev_get(dest);
> > +			if (rose_loopback_neigh->dev && dev) {
> >  				if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
> >  					kfree_skb(skb);
> >  			} else {
> > +				dev_put(dev);
> >  				kfree_skb(skb);
> >  			}
> >  		} else {
> 
> This is still not correct. With this code dev_put() could be called with
> NULL, which would cause a crash.
> 
> There is also a dev_put() missing if rose_rx_call_request() returns 0.
> 
> I think that this is the correct code:
> 
> diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
> index 7b094275ea8b..ff252ef73592 100644
> --- a/net/rose/rose_loopback.c
> +++ b/net/rose/rose_loopback.c
> @@ -96,11 +96,22 @@ static void rose_loopback_timer(struct timer_list *unused)
>  		}
>  
>  		if (frametype == ROSE_CALL_REQUEST) {
> -			if ((dev = rose_dev_get(dest)) != NULL) {
> -				if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
> -					kfree_skb(skb);
> -			} else {
> +			if (!rose_loopback_neigh->dev) {
>  				kfree_skb(skb);
> +				continue;
> +			}
> +
> +			dev = rose_dev_get(dest);
> +			if (!dev) {
> +				kfree_skb(skb);
> +				continue;
> +			}
> +
> +			if (rose_rx_call_request(skb, dev, rose_loopback_neigh,
> +						 lci_o) == 0) {
> +				dev_put(dev);
> +				kfree_skb(skb);
>  			}
>  		} else {
>  			kfree_skb(skb);
> 
> Please test this and resubmit it if it works.

Sure sir, I will test it and resend, if it works.


Thanks,
Anmol

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Linux-kernel-mentees] [PATCH v5 net] rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-15 11:44               ` Anmol Karn
@ 2020-11-19 19:10                 ` Anmol Karn
  2020-11-20 18:06                   ` Jakub Kicinski
  0 siblings, 1 reply; 13+ messages in thread
From: Anmol Karn @ 2020-11-19 19:10 UTC (permalink / raw)
  To: ralf, davem, kuba
  Cc: saeed, gregkh, netdev, linux-kernel, linux-hams,
	linux-kernel-mentees, syzkaller-bugs, anmol.karan123,
	syzbot+a1c743815982d9496393

rose_send_frame() dereferences `neigh->dev` when called from
rose_transmit_clear_request(), and the first occurrence of the
`neigh` is in rose_loopback_timer() as `rose_loopback_neigh`,
and it is initialized in rose_add_loopback_neigh() as NULL.
i.e when `rose_loopback_neigh` used in rose_loopback_timer()
its `->dev` was still NULL and rose_loopback_timer() was calling
rose_rx_call_request() without checking for NULL.

- net/rose/rose_link.c
This bug seems to get triggered in this line:

rose_call = (ax25_address *)neigh->dev->dev_addr;

Fix it by adding NULL checking for `rose_loopback_neigh->dev`
in rose_loopback_timer().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Reported-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
Tested-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>
---
Changes in v5:
	- Free `dev` in rose_rx_call_request() and add NULL check for `dev` before freeing it.
	(Suggested-by: Jakub Kicinski <kuba@kernel.org>)
Changes in v4:
	- Free `dev`(on dev_hold()), when neigh->dev is NULL. (Suggested-by: Jakub Kicinski <kuba@kernel.org>)
Changes in v3:
        - Corrected checkpatch warnings and errors (Suggested-by: Saeed Mahameed <saeed@kernel.org>)
        - Added "Fixes:" tag (Suggested-by: Saeed Mahameed <saeed@kernel.org>)
Changes in v2:
        - Added NULL check in rose_loopback_timer() (Suggested-by: Greg KH <gregkh@linuxfoundation.org>)

 net/rose/rose_loopback.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index 7b094275ea8b..11c45c8c6c16 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -96,10 +96,19 @@ static void rose_loopback_timer(struct timer_list *unused)
 		}

 		if (frametype == ROSE_CALL_REQUEST) {
-			if ((dev = rose_dev_get(dest)) != NULL) {
-				if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
-					kfree_skb(skb);
-			} else {
+			if (!rose_loopback_neigh->dev) {
+				kfree_skb(skb);
+				continue;
+			}
+
+			dev = rose_dev_get(dest);
+			if (!dev) {
+				kfree_skb(skb);
+				continue;
+			}
+
+			if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0) {
+				dev_put(dev);
 				kfree_skb(skb);
 			}
 		} else {
--
2.29.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v5 net] rose: Fix Null pointer dereference in rose_send_frame()
  2020-11-19 19:10                 ` [Linux-kernel-mentees] [PATCH v5 " Anmol Karn
@ 2020-11-20 18:06                   ` Jakub Kicinski
  0 siblings, 0 replies; 13+ messages in thread
From: Jakub Kicinski @ 2020-11-20 18:06 UTC (permalink / raw)
  To: Anmol Karn
  Cc: ralf, davem, saeed, gregkh, netdev, linux-kernel, linux-hams,
	linux-kernel-mentees, syzkaller-bugs,
	syzbot+a1c743815982d9496393

On Fri, 20 Nov 2020 00:40:43 +0530 Anmol Karn wrote:
> rose_send_frame() dereferences `neigh->dev` when called from
> rose_transmit_clear_request(), and the first occurrence of the
> `neigh` is in rose_loopback_timer() as `rose_loopback_neigh`,
> and it is initialized in rose_add_loopback_neigh() as NULL.
> i.e when `rose_loopback_neigh` used in rose_loopback_timer()
> its `->dev` was still NULL and rose_loopback_timer() was calling
> rose_rx_call_request() without checking for NULL.
> 
> - net/rose/rose_link.c
> This bug seems to get triggered in this line:
> 
> rose_call = (ax25_address *)neigh->dev->dev_addr;
> 
> Fix it by adding NULL checking for `rose_loopback_neigh->dev`
> in rose_loopback_timer().
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Reported-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
> Tested-by: syzbot+a1c743815982d9496393@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
> Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>

Applied to net, thanks!

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-11-20 18:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-05 15:56 [Linux-kernel-mentees] [PATCH v2 net] rose: Fix Null pointer dereference in rose_send_frame() Anmol Karn
2020-11-06 21:04 ` Saeed Mahameed
2020-11-07  8:20   ` Anmol Karn
2020-11-07 19:18     ` [Linux-kernel-mentees] [PATCH v3 " Anmol Karn
2020-11-10 17:58       ` Jakub Kicinski
2020-11-10 19:45         ` Anmol Karn
2020-11-11 16:59           ` [Linux-kernel-mentees] [PATCH v4 " Anmol Karn
2020-11-14 19:18             ` Jakub Kicinski
2020-11-15 11:44               ` Anmol Karn
2020-11-19 19:10                 ` [Linux-kernel-mentees] [PATCH v5 " Anmol Karn
2020-11-20 18:06                   ` Jakub Kicinski
2020-11-07 18:56 ` [Linux-kernel-mentees] [PATCH v2] net: " Anmol Karn
2020-11-07 19:02   ` Anmol karn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).