[Linux-kernel-mentees] [PATCH net] AX.25: Prevent out-of-bounds read in ax25_sendmsg()

[Linux-kernel-mentees] [PATCH net] AX.25: Prevent out-of-bounds read in ax25_sendmsg()

[Peilin Ye]

Checks on `addr_len` and `usax->sax25_ndigis` are insufficient.
ax25_sendmsg() can go out of bounds when `usax->sax25_ndigis` equals to 7
or 8. Fix it.

It is safe to remove `usax->sax25_ndigis > AX25_MAX_DIGIS`, since
`addr_len` is guaranteed to be less than or equal to
`sizeof(struct full_sockaddr_ax25)`

Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
 net/ax25/af_ax25.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index ef5bf116157a..0862fe49d434 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1509,7 +1509,8 @@ static int ax25_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 			struct full_sockaddr_ax25 *fsa = (struct full_sockaddr_ax25 *)usax;
 
 			/* Valid number of digipeaters ? */
-			if (usax->sax25_ndigis < 1 || usax->sax25_ndigis > AX25_MAX_DIGIS) {
+			if (usax->sax25_ndigis < 1 || addr_len < sizeof(struct sockaddr_ax25) +
+			    sizeof(ax25_address) * usax->sax25_ndigis) {
 				err = -EINVAL;
 				goto out;
 			}
-- 
2.25.1

[Linux-kernel-mentees] [PATCH net] AX.25: Prevent out-of-bounds read in ax25_sendmsg()

[Peilin Ye]

Checks on `addr_len` and `usax->sax25_ndigis` are insufficient.
ax25_sendmsg() can go out of bounds when `usax->sax25_ndigis` equals to 7
or 8. Fix it.

It is safe to remove `usax->sax25_ndigis > AX25_MAX_DIGIS`, since
`addr_len` is guaranteed to be less than or equal to
`sizeof(struct full_sockaddr_ax25)`

Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
 net/ax25/af_ax25.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index ef5bf116157a..0862fe49d434 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1509,7 +1509,8 @@ static int ax25_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 			struct full_sockaddr_ax25 *fsa = (struct full_sockaddr_ax25 *)usax;
 
 			/* Valid number of digipeaters ? */
-			if (usax->sax25_ndigis < 1 || usax->sax25_ndigis > AX25_MAX_DIGIS) {
+			if (usax->sax25_ndigis < 1 || addr_len < sizeof(struct sockaddr_ax25) +
+			    sizeof(ax25_address) * usax->sax25_ndigis) {
 				err = -EINVAL;
 				goto out;
 			}
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH net] AX.25: Prevent out-of-bounds read in ax25_sendmsg()

[David Miller]

From: Peilin Ye <yepeilin.cs@gmail.com>
Date: Wed, 22 Jul 2020 12:05:12 -0400

> Checks on `addr_len` and `usax->sax25_ndigis` are insufficient.
> ax25_sendmsg() can go out of bounds when `usax->sax25_ndigis` equals to 7
> or 8. Fix it.
> 
> It is safe to remove `usax->sax25_ndigis > AX25_MAX_DIGIS`, since
> `addr_len` is guaranteed to be less than or equal to
> `sizeof(struct full_sockaddr_ax25)`
> 
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>

Applied.

Re: [Linux-kernel-mentees] [PATCH net] AX.25: Prevent out-of-bounds read in ax25_sendmsg()

[David Miller]

From: Peilin Ye <yepeilin.cs@gmail.com>
Date: Wed, 22 Jul 2020 12:05:12 -0400

> Checks on `addr_len` and `usax->sax25_ndigis` are insufficient.
> ax25_sendmsg() can go out of bounds when `usax->sax25_ndigis` equals to 7
> or 8. Fix it.
> 
> It is safe to remove `usax->sax25_ndigis > AX25_MAX_DIGIS`, since
> `addr_len` is guaranteed to be less than or equal to
> `sizeof(struct full_sockaddr_ax25)`
> 
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>

Applied.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH net] AX.25: Prevent out-of-bounds read in ax25_sendmsg()

[David Ranch]

I need to ask the following question to the Linux kernel community as 
the AX25 amateur radio community is already having to
work around a few broken commits that were committed 4.1.22+:

   Is anyone actually _*testing*_ these proposed changes to make sure 
the AX.25 and related ecosystem still work afterwards?

I've personally tried multiple times to recruit some help to get some of 
these previous commits rolled back from some
recommended kernel people including even some the original authors like 
Alan Cox, etc without any success.  I fear that if
more commits come into the kernel without any testing, the whole AX.25 
stack will become toxic and unusable.

I am not a kernel class developer but I am *personally* willing to help 
out on the testing effort and even help setup regression
topologies (VMs, containers, whatever) if there is some place that they 
can be ideally run in a continuous delivery.

If there is anyone also willing to help fix some of these previous 
commits and get the AX.25 stack kernel back on track,
I do have a bunch of details of the commit details, details on why those 
committers THOUGHT they were a good idea, etc.

--David
KI6ZHD


On 07/22/2020 06:07 PM, David Miller wrote:
> From: Peilin Ye <yepeilin.cs@gmail.com>
> Date: Wed, 22 Jul 2020 12:05:12 -0400
>
>> Checks on `addr_len` and `usax->sax25_ndigis` are insufficient.
>> ax25_sendmsg() can go out of bounds when `usax->sax25_ndigis` equals to 7
>> or 8. Fix it.
>>
>> It is safe to remove `usax->sax25_ndigis > AX25_MAX_DIGIS`, since
>> `addr_len` is guaranteed to be less than or equal to
>> `sizeof(struct full_sockaddr_ax25)`
>>
>> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> Applied.

Re: [Linux-kernel-mentees] [PATCH net] AX.25: Prevent out-of-bounds read in ax25_sendmsg()

[David Ranch]

I need to ask the following question to the Linux kernel community as 
the AX25 amateur radio community is already having to
work around a few broken commits that were committed 4.1.22+:

   Is anyone actually _*testing*_ these proposed changes to make sure 
the AX.25 and related ecosystem still work afterwards?

I've personally tried multiple times to recruit some help to get some of 
these previous commits rolled back from some
recommended kernel people including even some the original authors like 
Alan Cox, etc without any success.  I fear that if
more commits come into the kernel without any testing, the whole AX.25 
stack will become toxic and unusable.

I am not a kernel class developer but I am *personally* willing to help 
out on the testing effort and even help setup regression
topologies (VMs, containers, whatever) if there is some place that they 
can be ideally run in a continuous delivery.

If there is anyone also willing to help fix some of these previous 
commits and get the AX.25 stack kernel back on track,
I do have a bunch of details of the commit details, details on why those 
committers THOUGHT they were a good idea, etc.

--David
KI6ZHD


On 07/22/2020 06:07 PM, David Miller wrote:
> From: Peilin Ye <yepeilin.cs@gmail.com>
> Date: Wed, 22 Jul 2020 12:05:12 -0400
>
>> Checks on `addr_len` and `usax->sax25_ndigis` are insufficient.
>> ax25_sendmsg() can go out of bounds when `usax->sax25_ndigis` equals to 7
>> or 8. Fix it.
>>
>> It is safe to remove `usax->sax25_ndigis > AX25_MAX_DIGIS`, since
>> `addr_len` is guaranteed to be less than or equal to
>> `sizeof(struct full_sockaddr_ax25)`
>>
>> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> Applied.

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees