* linux mkiss.c panic: fix
@ 2017-02-09 9:27 Thomas Osterried
2017-02-09 13:12 ` [PATCH] NET: mkiss: Fix panic Ralf Baechle
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Osterried @ 2017-02-09 9:27 UTC (permalink / raw)
To: netdev, David S. Miller, Andrew Morton, linux-kernel
Cc: Ralf Bächle DL5RB
Hello,
linux mkiss driver could cause a panic.
This patch fixes this issue:
Signed-off-by: Thomas Osterried <thomas@osterried.de>
mkiss kernel panic fix: Correct device variables guarantees proper skb room.
--- mkiss.c.orig 2015-11-18 11:08:46.000000000 +0100
+++ mkiss.c 2016-02-06 18:04:05.000000000 +0100
@@ -678,8 +678,8 @@
{
/* Finish setting up the DEVICE info. */
dev->mtu = AX_MTU;
- dev->hard_header_len = 0;
- dev->addr_len = 0;
+ dev->hard_header_len = AX25_MAX_HEADER_LEN;
+ dev->addr_len = AX25_ADDR_LEN;
dev->type = ARPHRD_AX25;
dev->tx_queue_len = 10;
dev->header_ops = &ax_header_ops;
Reason:
if you plug off i.e. your usb-serial-adapter, the driver re-initializes, with dev->hard_header_len and dev->addr_len set to zero, instead of the correct values.
If afterwards a packet should be sent to the half-dead interface, it causes a kernel panic.
These device parameters are used in other parts of the IP-stack when calculating the necessary room for the skb. After a packet goes to the mkiss driver for being sent out, there's no room left in the skb, due to the reserved length of 0. If skb_push pushes the ax25-header to the skb with no room left, we panic.
The panic looked like this:
=>
>> [<c0595468>] (skb_panic) from [<c0401f70>] (skb_push+0x4c/0x50)
>> [<c0401f70>] (skb_push) from [<bf0bdad4>] (ax25_hard_header+0x34/0xf4 [ax25])
>> [<bf0bdad4>] (ax25_hard_header [ax25]) from [<bf0d05d4>] (ax_header+0x38/0x40 [mkiss])
>> [<bf0d05d4>] (ax_header [mkiss]) from [<c041b584>] (neigh_compat_output+0x8c/0xd8)
>> [<c041b584>] (neigh_compat_output) from [<c043e7a8>] (ip_finish_output+0x2a0/0x914)
>> [<c043e7a8>] (ip_finish_output) from [<c043f948>] (ip_output+0xd8/0xf0)
>> [<c043f948>] (ip_output) from [<c043f04c>] (ip_local_out_sk+0x44/0x48)
This patch makes mkiss behave like the 6pack driver. 6pack does not panic.
In 6pack.c sp_setup() (same function name here) the values for dev->hard_header_len and dev->addr_len are set to the same values as in my mkiss patch.
vy 73,
- Thomas dl9sau
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] NET: mkiss: Fix panic
2017-02-09 9:27 linux mkiss.c panic: fix Thomas Osterried
@ 2017-02-09 13:12 ` Ralf Baechle
2017-02-10 18:42 ` David Miller
2017-02-10 23:01 ` [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl Ralf Baechle
0 siblings, 2 replies; 6+ messages in thread
From: Ralf Baechle @ 2017-02-09 13:12 UTC (permalink / raw)
To: netdev, David S. Miller, Andrew Morton, linux-kernel; +Cc: Thomas Osterried
If a USB-to-serial adapter is unplugged, the driver re-initializes, with
dev->hard_header_len and dev->addr_len set to zero, instead of the correct
values. If then a packet is sent through the half-dead interface, the
kernel will panic due to running out of headroom in the skb when pushing
for the AX.25 headers resulting in this panic:
[<c0595468>] (skb_panic) from [<c0401f70>] (skb_push+0x4c/0x50)
[<c0401f70>] (skb_push) from [<bf0bdad4>] (ax25_hard_header+0x34/0xf4 [ax25])
[<bf0bdad4>] (ax25_hard_header [ax25]) from [<bf0d05d4>] (ax_header+0x38/0x40 [mkiss])
[<bf0d05d4>] (ax_header [mkiss]) from [<c041b584>] (neigh_compat_output+0x8c/0xd8)
[<c041b584>] (neigh_compat_output) from [<c043e7a8>] (ip_finish_output+0x2a0/0x914)
[<c043e7a8>] (ip_finish_output) from [<c043f948>] (ip_output+0xd8/0xf0)
[<c043f948>] (ip_output) from [<c043f04c>] (ip_local_out_sk+0x44/0x48)
This patch makes mkiss behave like the 6pack driver. 6pack does not
panic. In 6pack.c sp_setup() (same function name here) the values for
dev->hard_header_len and dev->addr_len are set to the same values as in
my mkiss patch.
[ralf@linux-mips.org: Massages original submission to conform to the usual
standards for patch submissions.]
Signed-off-by: Thomas Osterried <thomas@osterried.de>
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
---
drivers/net/hamradio/mkiss.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
index 1dfe230..e0a6b1a 100644
--- a/drivers/net/hamradio/mkiss.c
+++ b/drivers/net/hamradio/mkiss.c
@@ -648,8 +648,8 @@ static void ax_setup(struct net_device *dev)
{
/* Finish setting up the DEVICE info. */
dev->mtu = AX_MTU;
- dev->hard_header_len = 0;
- dev->addr_len = 0;
+ dev->hard_header_len = AX25_MAX_HEADER_LEN;
+ dev->addr_len = AX25_ADDR_LEN;
dev->type = ARPHRD_AX25;
dev->tx_queue_len = 10;
dev->header_ops = &ax25_header_ops;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] NET: mkiss: Fix panic
2017-02-09 13:12 ` [PATCH] NET: mkiss: Fix panic Ralf Baechle
@ 2017-02-10 18:42 ` David Miller
2017-02-10 23:01 ` [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl Ralf Baechle
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2017-02-10 18:42 UTC (permalink / raw)
To: ralf; +Cc: netdev, akpm, linux-kernel, thomas
From: Ralf Baechle <ralf@linux-mips.org>
Date: Thu, 9 Feb 2017 14:12:11 +0100
> If a USB-to-serial adapter is unplugged, the driver re-initializes, with
> dev->hard_header_len and dev->addr_len set to zero, instead of the correct
> values. If then a packet is sent through the half-dead interface, the
> kernel will panic due to running out of headroom in the skb when pushing
> for the AX.25 headers resulting in this panic:
>
> [<c0595468>] (skb_panic) from [<c0401f70>] (skb_push+0x4c/0x50)
> [<c0401f70>] (skb_push) from [<bf0bdad4>] (ax25_hard_header+0x34/0xf4 [ax25])
> [<bf0bdad4>] (ax25_hard_header [ax25]) from [<bf0d05d4>] (ax_header+0x38/0x40 [mkiss])
> [<bf0d05d4>] (ax_header [mkiss]) from [<c041b584>] (neigh_compat_output+0x8c/0xd8)
> [<c041b584>] (neigh_compat_output) from [<c043e7a8>] (ip_finish_output+0x2a0/0x914)
> [<c043e7a8>] (ip_finish_output) from [<c043f948>] (ip_output+0xd8/0xf0)
> [<c043f948>] (ip_output) from [<c043f04c>] (ip_local_out_sk+0x44/0x48)
>
> This patch makes mkiss behave like the 6pack driver. 6pack does not
> panic. In 6pack.c sp_setup() (same function name here) the values for
> dev->hard_header_len and dev->addr_len are set to the same values as in
> my mkiss patch.
>
> [ralf@linux-mips.org: Massages original submission to conform to the usual
> standards for patch submissions.]
>
> Signed-off-by: Thomas Osterried <thomas@osterried.de>
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
Applied, thank you.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl
2017-02-09 13:12 ` [PATCH] NET: mkiss: Fix panic Ralf Baechle
2017-02-10 18:42 ` David Miller
@ 2017-02-10 23:01 ` Ralf Baechle
2017-02-11 10:53 ` walter harms
1 sibling, 1 reply; 6+ messages in thread
From: Ralf Baechle @ 2017-02-10 23:01 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: Thomas Osterried, linux-hams
When looking at Thomas' mkiss fix 7ba1b6890387 ("NET: mkiss: Fix panic")
I noticed that the mkiss SIOCSIFENCAPS ioctl was also doing a slightly
strange assignment
dev->hard_header_len = AX25_KISS_HEADER_LEN +
AX25_MAX_HEADER_LEN + 3;
AX25_MAX_HEADER_LEN already accounts for the KISS byte so adding
AX25_KISS_HEADER_LEN is a double allocation nor does the "+ 3" seem to
be necessary. So this can be simplified to
dev->hard_header_len = AX25_MAX_HEADER_LEN
which after the preceeding fix is a redundant assignment of what
ax_setup has already assigned so delete the line. The assignments
to dev->addr_len and dev->type are similarly redundant.
The SIOCSIFENCAP argument was never checked for validity. Check that
it is 4 and return -EINVAL if not. The magic constant 4 dates back to
the days when KISS was handled by the SLIP driver where it had the
symbol name SL_MODE_AX25.
Since however mkiss.c only supports a single encapsulation mode there
is no point in storing it in struct mkiss so delete all that.
Note that while useless we can't delete the SIOCSIFENCAP ioctl as
kissattach(8) is still using it and without mkiss issuing a
SIOCSIFENCAP ioctl an older kernel that does not have Thomas' mkiss fix
would still panic on attempt to transmit via mkiss.
6pack was suffering from the same issue except there SIOCGIFENCAP was
return 0 for the encapsulation while the spattach utility was passing 4
for the mode, so the mode check added for 6pack is a bit more lenient
allow the values 0 and 4 to be set. That way we retain the option
to set different encapsulation modes for future extensions.
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
drivers/net/hamradio/6pack.c | 10 ++++------
drivers/net/hamradio/mkiss.c | 10 ++++------
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index 470b3dc..d949b9f 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c
@@ -104,7 +104,6 @@ struct sixpack {
int buffsize; /* Max buffers sizes */
unsigned long flags; /* Flag values/ mode etc */
- unsigned char mode; /* 6pack mode */
/* 6pack stuff */
unsigned char tx_delay;
@@ -723,11 +722,10 @@ static int sixpack_ioctl(struct tty_struct *tty, struct file *file,
break;
}
- sp->mode = tmp;
- dev->addr_len = AX25_ADDR_LEN;
- dev->hard_header_len = AX25_KISS_HEADER_LEN +
- AX25_MAX_HEADER_LEN + 3;
- dev->type = ARPHRD_AX25;
+ if (tmp != 0 && tmp != 4) {
+ err = -EINVAL;
+ break;
+ }
err = 0;
break;
diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
index 1dfe230..cdaf819 100644
--- a/drivers/net/hamradio/mkiss.c
+++ b/drivers/net/hamradio/mkiss.c
@@ -71,7 +71,6 @@ struct mkiss {
#define AXF_KEEPTEST 3 /* Keepalive test flag */
#define AXF_OUTWAIT 4 /* is outpacket was flag */
- int mode;
int crcmode; /* MW: for FlexNet, SMACK etc. */
int crcauto; /* CRC auto mode */
@@ -841,11 +840,10 @@ static int mkiss_ioctl(struct tty_struct *tty, struct file *file,
break;
}
- ax->mode = tmp;
- dev->addr_len = AX25_ADDR_LEN;
- dev->hard_header_len = AX25_KISS_HEADER_LEN +
- AX25_MAX_HEADER_LEN + 3;
- dev->type = ARPHRD_AX25;
+ if (tmp != 4) {
+ err = -EINVAL;
+ break;
+ }
err = 0;
break;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl
2017-02-10 23:01 ` [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl Ralf Baechle
@ 2017-02-11 10:53 ` walter harms
2017-02-12 2:30 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: walter harms @ 2017-02-11 10:53 UTC (permalink / raw)
To: Ralf Baechle; +Cc: David S. Miller, netdev, Thomas Osterried, linux-hams
Am 11.02.2017 00:01, schrieb Ralf Baechle:
> When looking at Thomas' mkiss fix 7ba1b6890387 ("NET: mkiss: Fix panic")
> I noticed that the mkiss SIOCSIFENCAPS ioctl was also doing a slightly
> strange assignment
>
> dev->hard_header_len = AX25_KISS_HEADER_LEN +
> AX25_MAX_HEADER_LEN + 3;
>
> AX25_MAX_HEADER_LEN already accounts for the KISS byte so adding
> AX25_KISS_HEADER_LEN is a double allocation nor does the "+ 3" seem to
> be necessary. So this can be simplified to
>
> dev->hard_header_len = AX25_MAX_HEADER_LEN
>
> which after the preceeding fix is a redundant assignment of what
> ax_setup has already assigned so delete the line. The assignments
> to dev->addr_len and dev->type are similarly redundant.
>
> The SIOCSIFENCAP argument was never checked for validity. Check that
> it is 4 and return -EINVAL if not. The magic constant 4 dates back to
> the days when KISS was handled by the SLIP driver where it had the
> symbol name SL_MODE_AX25.
>
> Since however mkiss.c only supports a single encapsulation mode there
> is no point in storing it in struct mkiss so delete all that.
>
> Note that while useless we can't delete the SIOCSIFENCAP ioctl as
> kissattach(8) is still using it and without mkiss issuing a
> SIOCSIFENCAP ioctl an older kernel that does not have Thomas' mkiss fix
> would still panic on attempt to transmit via mkiss.
>
> 6pack was suffering from the same issue except there SIOCGIFENCAP was
> return 0 for the encapsulation while the spattach utility was passing 4
> for the mode, so the mode check added for 6pack is a bit more lenient
> allow the values 0 and 4 to be set. That way we retain the option
> to set different encapsulation modes for future extensions.
>
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
>
> drivers/net/hamradio/6pack.c | 10 ++++------
> drivers/net/hamradio/mkiss.c | 10 ++++------
> 2 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
> index 470b3dc..d949b9f 100644
> --- a/drivers/net/hamradio/6pack.c
> +++ b/drivers/net/hamradio/6pack.c
> @@ -104,7 +104,6 @@ struct sixpack {
> int buffsize; /* Max buffers sizes */
>
> unsigned long flags; /* Flag values/ mode etc */
> - unsigned char mode; /* 6pack mode */
>
> /* 6pack stuff */
> unsigned char tx_delay;
> @@ -723,11 +722,10 @@ static int sixpack_ioctl(struct tty_struct *tty, struct file *file,
> break;
> }
>
> - sp->mode = tmp;
> - dev->addr_len = AX25_ADDR_LEN;
> - dev->hard_header_len = AX25_KISS_HEADER_LEN +
> - AX25_MAX_HEADER_LEN + 3;
> - dev->type = ARPHRD_AX25;
> + if (tmp != 0 && tmp != 4) {
> + err = -EINVAL;
> + break;
> + }
>
What is about a comment like:
/*
The magic constant 4 dates back to the days when KISS was handled by the SLIP driver where it had the
symbol name SL_MODE_AX25.
*/
just not to confuse future readers ..
re,
wh
> err = 0;
> break;
> diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
> index 1dfe230..cdaf819 100644
> --- a/drivers/net/hamradio/mkiss.c
> +++ b/drivers/net/hamradio/mkiss.c
> @@ -71,7 +71,6 @@ struct mkiss {
> #define AXF_KEEPTEST 3 /* Keepalive test flag */
> #define AXF_OUTWAIT 4 /* is outpacket was flag */
>
> - int mode;
> int crcmode; /* MW: for FlexNet, SMACK etc. */
> int crcauto; /* CRC auto mode */
>
> @@ -841,11 +840,10 @@ static int mkiss_ioctl(struct tty_struct *tty, struct file *file,
> break;
> }
>
> - ax->mode = tmp;
> - dev->addr_len = AX25_ADDR_LEN;
> - dev->hard_header_len = AX25_KISS_HEADER_LEN +
> - AX25_MAX_HEADER_LEN + 3;
> - dev->type = ARPHRD_AX25;
> + if (tmp != 4) {
> + err = -EINVAL;
> + break;
> + }
>
> err = 0;
> break;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hams" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl
2017-02-11 10:53 ` walter harms
@ 2017-02-12 2:30 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2017-02-12 2:30 UTC (permalink / raw)
To: wharms; +Cc: ralf, netdev, thomas, linux-hams
From: walter harms <wharms@bfs.de>
Date: Sat, 11 Feb 2017 11:53:09 +0100
>
>
> Am 11.02.2017 00:01, schrieb Ralf Baechle:
>> When looking at Thomas' mkiss fix 7ba1b6890387 ("NET: mkiss: Fix panic")
>> I noticed that the mkiss SIOCSIFENCAPS ioctl was also doing a slightly
>> strange assignment
>>
>> dev->hard_header_len = AX25_KISS_HEADER_LEN +
>> AX25_MAX_HEADER_LEN + 3;
>>
>> AX25_MAX_HEADER_LEN already accounts for the KISS byte so adding
>> AX25_KISS_HEADER_LEN is a double allocation nor does the "+ 3" seem to
>> be necessary. So this can be simplified to
>>
>> dev->hard_header_len = AX25_MAX_HEADER_LEN
>>
>> which after the preceeding fix is a redundant assignment of what
>> ax_setup has already assigned so delete the line. The assignments
>> to dev->addr_len and dev->type are similarly redundant.
>>
>> The SIOCSIFENCAP argument was never checked for validity. Check that
>> it is 4 and return -EINVAL if not. The magic constant 4 dates back to
>> the days when KISS was handled by the SLIP driver where it had the
>> symbol name SL_MODE_AX25.
>>
>> Since however mkiss.c only supports a single encapsulation mode there
>> is no point in storing it in struct mkiss so delete all that.
>>
>> Note that while useless we can't delete the SIOCSIFENCAP ioctl as
>> kissattach(8) is still using it and without mkiss issuing a
>> SIOCSIFENCAP ioctl an older kernel that does not have Thomas' mkiss fix
>> would still panic on attempt to transmit via mkiss.
>>
>> 6pack was suffering from the same issue except there SIOCGIFENCAP was
>> return 0 for the encapsulation while the spattach utility was passing 4
>> for the mode, so the mode check added for 6pack is a bit more lenient
>> allow the values 0 and 4 to be set. That way we retain the option
>> to set different encapsulation modes for future extensions.
>>
>> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
>>
>> drivers/net/hamradio/6pack.c | 10 ++++------
>> drivers/net/hamradio/mkiss.c | 10 ++++------
>> 2 files changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
>> index 470b3dc..d949b9f 100644
>> --- a/drivers/net/hamradio/6pack.c
>> +++ b/drivers/net/hamradio/6pack.c
>> @@ -104,7 +104,6 @@ struct sixpack {
>> int buffsize; /* Max buffers sizes */
>>
>> unsigned long flags; /* Flag values/ mode etc */
>> - unsigned char mode; /* 6pack mode */
>>
>> /* 6pack stuff */
>> unsigned char tx_delay;
>> @@ -723,11 +722,10 @@ static int sixpack_ioctl(struct tty_struct *tty, struct file *file,
>> break;
>> }
>>
>> - sp->mode = tmp;
>> - dev->addr_len = AX25_ADDR_LEN;
>> - dev->hard_header_len = AX25_KISS_HEADER_LEN +
>> - AX25_MAX_HEADER_LEN + 3;
>> - dev->type = ARPHRD_AX25;
>> + if (tmp != 0 && tmp != 4) {
>> + err = -EINVAL;
>> + break;
>> + }
>>
>
> What is about a comment like:
> /*
> The magic constant 4 dates back to the days when KISS was handled by the SLIP driver where it had the
> symbol name SL_MODE_AX25.
> */
>
> just not to confuse future readers ..
Agreed, every magic comment needs a define or a big comment.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-12 2:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-09 9:27 linux mkiss.c panic: fix Thomas Osterried
2017-02-09 13:12 ` [PATCH] NET: mkiss: Fix panic Ralf Baechle
2017-02-10 18:42 ` David Miller
2017-02-10 23:01 ` [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl Ralf Baechle
2017-02-11 10:53 ` walter harms
2017-02-12 2:30 ` David Miller
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).