net/socket: Ensure length of input socket option param >= sizeof(int)

Commit Message

Edward Adam Davis April 9, 2024, 12:15 p.m. UTC

The optlen value passed by syzbot to _sys_setsockopt() is 2, which results in
only 2 bytes being allocated when allocating memory to kernel_optval, and the
optval size passed when calling the function copy_from_sockptr() is 4 bytes.
Here, optlen is determined uniformly in the entry function __sys_setsockopt(). 
If its value is less than 4, the parameter is considered invalid.

Reported-by: [e-mail]
Reported-by: [e-mail]
Reported-by: [e-mail]
Signed-off-by: Edward Adam Davis <[e-mail]>
---
 net/socket.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Eric Dumazet April 9, 2024, 1:07 p.m. UTC | #1

On 4/9/24 14:15, Edward Adam Davis wrote:
> The optlen value passed by syzbot to _sys_setsockopt() is 2, which results in
> only 2 bytes being allocated when allocating memory to kernel_optval, and the
> optval size passed when calling the function copy_from_sockptr() is 4 bytes.
> Here, optlen is determined uniformly in the entry function __sys_setsockopt().
> If its value is less than 4, the parameter is considered invalid.
>
> Reported-by: [e-mail]
> Reported-by: [e-mail]
> Reported-by: [e-mail]
> Signed-off-by: Edward Adam Davis <[e-mail]>


I think I gave my feedback already.

Please do not ignore maintainers feedback.

This patch is absolutely wrong.

Some setsockopt() deal with optlen == 1 just fine, thank you very much.

[e-mail] April 9, 2024, 1:08 p.m. UTC | #2

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=842803

---Test result---

Test Summary:
CheckPatch                    FAIL      0.88 seconds
GitLint                       FAIL      0.46 seconds
SubjectPrefix                 FAIL      0.43 seconds
BuildKernel                   PASS      31.15 seconds
CheckAllWarning               PASS      33.29 seconds
CheckSparse                   PASS      38.65 seconds
CheckSmatch                   FAIL      35.17 seconds
BuildKernel32                 PASS      29.25 seconds
TestRunnerSetup               PASS      532.15 seconds
TestRunner_l2cap-tester       FAIL      17.25 seconds
TestRunner_iso-tester         FAIL      35.88 seconds
TestRunner_bnep-tester        PASS      4.74 seconds
TestRunner_mgmt-tester        FAIL      113.82 seconds
TestRunner_rfcomm-tester      PASS      7.51 seconds
TestRunner_sco-tester         FAIL      15.66 seconds
TestRunner_ioctl-tester       PASS      7.73 seconds
TestRunner_mesh-tester        PASS      5.73 seconds
TestRunner_smp-tester         PASS      6.71 seconds
TestRunner_userchan-tester    PASS      4.79 seconds
IncrementalBuild              PASS      28.18 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
net/socket: Ensure length of input socket option param >= sizeof(int)
WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#86: 
The optlen value passed by syzbot to _sys_setsockopt() is 2, which results in

WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#92: 
Reported-by: [e-mail]
Reported-by: [e-mail]

WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#93: 
Reported-by: [e-mail]
Reported-by: [e-mail]

WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#94: 
Reported-by: [e-mail]
Signed-off-by: Edward Adam Davis <[e-mail]>

total: 0 errors, 4 warnings, 0 checks, 9 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13622424.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
net/socket: Ensure length of input socket option param >= sizeof(int)

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
6: B2 Line has trailing whitespace: "Here, optlen is determined uniformly in the entry function __sys_setsockopt(). "
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_l2cap-tester - FAIL
Desc: Run l2cap-tester with test-runner
Output:
Total: 55, Passed: 40 (72.7%), Failed: 15, Not Run: 0

Failed Test Cases
L2CAP BR/EDR Client SSP - Success 2                  Failed       0.072 seconds
L2CAP BR/EDR Client PIN Code - Success               Failed       0.057 seconds
L2CAP LE Client SMP - Success                        Failed       0.060 seconds
L2CAP Ext-Flowctl Client - Success                   Failed       0.056 seconds
L2CAP Ext-Flowctl Client - Close                     Failed       0.064 seconds
L2CAP Ext-Flowctl Client - Timeout                   Failed       0.061 seconds
L2CAP Ext-Flowctl Client, Direct Advertising - Success Failed       0.063 seconds
L2CAP Ext-Flowctl Client SMP - Success               Failed       0.069 seconds
L2CAP Ext-Flowctl Client - Command Reject            Failed       0.064 seconds
L2CAP Ext-Flowctl Client - Open two sockets          Failed       0.061 seconds
L2CAP Ext-Flowctl Client - Open two sockets close one Failed       0.066 seconds
L2CAP LE ATT Client - Success                        Failed       0.063 seconds
L2CAP LE EATT Client - Success                       Failed       0.064 seconds
L2CAP LE EATT Server - Success                       Failed       0.060 seconds
L2CAP LE EATT Server - Reject                        Failed       0.059 seconds
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
Total: 121, Passed: 120 (99.2%), Failed: 1, Not Run: 0

Failed Test Cases
ISO Connect Suspend - Success                        Failed       4.164 seconds
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2

Failed Test Cases
LL Privacy - Add Device 5 (2 Devices to RL)          Failed       0.171 seconds
##############################
Test: TestRunner_sco-tester - FAIL
Desc: Run sco-tester with test-runner
Output:
Total: 15, Passed: 12 (80.0%), Failed: 3, Not Run: 0

Failed Test Cases
Basic SCO Set Socket Option - Success                Failed       0.085 seconds
eSCO mSBC - Success                                  Failed       0.078 seconds
SCO mSBC 1.1 - Failure                               Failed       0.079 seconds


---
Regards,
Linux Bluetooth

Luiz Augusto von Dentz April 9, 2024, 1:25 p.m. UTC | #3

Hi,

On Tue, Apr 9, 2024 at 9:07 AM Eric Dumazet <[e-mail]> wrote:
>
>
> On 4/9/24 14:15, Edward Adam Davis wrote:
> > The optlen value passed by syzbot to _sys_setsockopt() is 2, which results in
> > only 2 bytes being allocated when allocating memory to kernel_optval, and the
> > optval size passed when calling the function copy_from_sockptr() is 4 bytes.
> > Here, optlen is determined uniformly in the entry function __sys_setsockopt().
> > If its value is less than 4, the parameter is considered invalid.
> >
> > Reported-by: [e-mail]
> > Reported-by: [e-mail]
> > Reported-by: [e-mail]
> > Signed-off-by: Edward Adam Davis <[e-mail]>
>
>
> I think I gave my feedback already.
>
> Please do not ignore maintainers feedback.
>
> This patch is absolutely wrong.
>
> Some setsockopt() deal with optlen == 1 just fine, thank you very much.

+1, I don't think the setsockopt interface has a fixed minimum of
sizeof(int), so this is a nak from me as well.

Edward Adam Davis April 9, 2024, 2:01 p.m. UTC | #4

On Tue, 9 Apr 2024 15:07:41 +0200, Eric Dumazet wrote:
> > The optlen value passed by syzbot to _sys_setsockopt() is 2, which results in
> > only 2 bytes being allocated when allocating memory to kernel_optval, and the
> > optval size passed when calling the function copy_from_sockptr() is 4 bytes.
> > Here, optlen is determined uniformly in the entry function __sys_setsockopt().
> > If its value is less than 4, the parameter is considered invalid.
> >
> > Reported-by: [e-mail]
> > Reported-by: [e-mail]
> > Reported-by: [e-mail]
> > Signed-off-by: Edward Adam Davis <[e-mail]>
> 
> 
> I think I gave my feedback already.
> 
> Please do not ignore maintainers feedback.
> 
> This patch is absolutely wrong.
> 
> Some setsockopt() deal with optlen == 1 just fine, thank you very much.
It's better to use evidence to support your claim, rather than your "maintainer" title.

Eric Dumazet April 9, 2024, 2:17 p.m. UTC | #5

On Tue, Apr 9, 2024 at 4:02 PM Edward Adam Davis <[e-mail]> wrote:
>
> On Tue, 9 Apr 2024 15:07:41 +0200, Eric Dumazet wrote:
> > > The optlen value passed by syzbot to _sys_setsockopt() is 2, which results in
> > > only 2 bytes being allocated when allocating memory to kernel_optval, and the
> > > optval size passed when calling the function copy_from_sockptr() is 4 bytes.
> > > Here, optlen is determined uniformly in the entry function __sys_setsockopt().
> > > If its value is less than 4, the parameter is considered invalid.
> > >
> > > Reported-by: [e-mail]
> > > Reported-by: [e-mail]
> > > Reported-by: [e-mail]
> > > Signed-off-by: Edward Adam Davis <[e-mail]>
> >
> >
> > I think I gave my feedback already.
> >
> > Please do not ignore maintainers feedback.
> >
> > This patch is absolutely wrong.
> >
> > Some setsockopt() deal with optlen == 1 just fine, thank you very much.
> It's better to use evidence to support your claim, rather than your "maintainer" title.

I will answer since you ask so nicely,
but if you plan sending linux kernel patches, I suggest you look in
the source code.

Look at do_ip_setsockopt(), which is one of the most used setsockopt()
in the world.

The code is at least 20 years old.

It even supports optlen == 0

               if (optlen >= sizeof(int)) {
                       if (copy_from_sockptr(&val, optval, sizeof(val)))
                               return -EFAULT;
               } else if (optlen >= sizeof(char)) {
                       unsigned char ucval;

                       if (copy_from_sockptr(&ucval, optval, sizeof(ucval)))
                               return -EFAULT;
                       val = (int) ucval;
               }
       }

       /* If optlen==0, it is equivalent to val == 0 */

Jakub Kicinski April 9, 2024, 6:27 p.m. UTC | #6

On Tue,  9 Apr 2024 22:01:45 +0800 Edward Adam Davis wrote:
> > I think I gave my feedback already.
> > 
> > Please do not ignore maintainers feedback.
> > 
> > This patch is absolutely wrong.
> > 
> > Some setsockopt() deal with optlen == 1 just fine, thank you very much.  
>
> It's better to use evidence to support your claim, rather than your "maintainer" title.

Run selftests.

Patch

diff --git a/net/socket.c b/net/socket.c
index e5f3af49a8b6..ac8fd4f6ebfe 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2327,6 +2327,9 @@  int __sys_setsockopt(int fd, int level, int optname, char __user *user_optval,
 	int err, fput_needed;
 	struct socket *sock;
 
+	if (optlen < sizeof(int))
+		return -EINVAL;
+
 	sock = sockfd_lookup_light(fd, &err, &fput_needed);
 	if (!sock)
 		return err;