linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* RE: net: Fix some callers of copy_from_sockptr()
  2024-11-14 23:27 [PATCH net 1/4] bluetooth: Improve setsockopt() handling of malformed user input Michal Luczaj
@ 2024-11-15  1:20 ` bluez.test.bot
  0 siblings, 0 replies; 10+ messages in thread
From: bluez.test.bot @ 2024-11-15  1:20 UTC (permalink / raw)
  To: linux-bluetooth, mhal

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

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=909786

---Test result---

Test Summary:
CheckPatch                    PENDING   0.24 seconds
GitLint                       PENDING   0.20 seconds
SubjectPrefix                 FAIL      1.33 seconds
BuildKernel                   PASS      24.84 seconds
CheckAllWarning               PASS      27.20 seconds
CheckSparse                   WARNING   30.92 seconds
BuildKernel32                 PASS      24.40 seconds
TestRunnerSetup               PASS      435.43 seconds
TestRunner_l2cap-tester       PASS      23.36 seconds
TestRunner_iso-tester         FAIL      34.58 seconds
TestRunner_bnep-tester        PASS      4.81 seconds
TestRunner_mgmt-tester        PASS      120.44 seconds
TestRunner_rfcomm-tester      PASS      7.62 seconds
TestRunner_sco-tester         PASS      11.39 seconds
TestRunner_ioctl-tester       PASS      8.05 seconds
TestRunner_mesh-tester        PASS      6.01 seconds
TestRunner_smp-tester         PASS      7.97 seconds
TestRunner_userchan-tester    PASS      5.02 seconds
IncrementalBuild              PENDING   0.47 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
Total: 124, Passed: 119 (96.0%), Failed: 1, Not Run: 4

Failed Test Cases
ISO Connect2 Suspend - Success                       Failed       4.238 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth


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

* [PATCH net v2 0/4] net: Fix some callers of copy_from_sockptr()
@ 2024-11-15 13:21 Michal Luczaj
  2024-11-15 13:21 ` [PATCH net v2 1/4] Bluetooth: Improve setsockopt() handling of malformed user input Michal Luczaj
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Michal Luczaj @ 2024-11-15 13:21 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
	David S. Miller, Eric Dumazet, Paolo Abeni, Simon Horman,
	David Howells, Marc Dionne
  Cc: Luiz Augusto von Dentz, linux-bluetooth, netdev, linux-afs,
	Jakub Kicinski, Michal Luczaj

Some callers misinterpret copy_from_sockptr()'s return value. The function
follows copy_from_user(), i.e. returns 0 for success, or the number of
bytes not copied on error. Simply returning the result in a non-zero case
isn't usually what was intended.

Compile tested with CONFIG_LLC, CONFIG_AF_RXRPC, CONFIG_BT enabled.

Last patch probably belongs more to net-next, if any. Here as a RFC.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Changes in v2:
- Fix the fix of llc_ui_setsockopt()
- Switch "bluetooth:" to "Bluetooth:" [bluez.test.bot]
- Collect Reviewed-by [Luiz Augusto von Dentz]
- Link to v1: https://lore.kernel.org/r/20241115-sockptr-copy-fixes-v1-0-d183c87fcbd5@rbox.co

---
Michal Luczaj (4):
      Bluetooth: Improve setsockopt() handling of malformed user input
      llc: Improve setsockopt() handling of malformed user input
      rxrpc: Improve setsockopt() handling of malformed user input
      net: Comment copy_from_sockptr() explaining its behaviour

 include/linux/sockptr.h           |  2 ++
 include/net/bluetooth/bluetooth.h |  9 ---------
 net/bluetooth/hci_sock.c          | 14 +++++++-------
 net/bluetooth/iso.c               | 10 +++++-----
 net/bluetooth/l2cap_sock.c        | 20 +++++++++++---------
 net/bluetooth/rfcomm/sock.c       |  9 ++++-----
 net/bluetooth/sco.c               | 11 ++++++-----
 net/llc/af_llc.c                  |  8 ++++----
 net/rxrpc/af_rxrpc.c              |  8 ++++----
 9 files changed, 43 insertions(+), 48 deletions(-)
---
base-commit: ea301aec8bb718b02b68761d2229fc12c9fefa29
change-id: 20241114-sockptr-copy-fixes-3999eaa81aa1

Best regards,
-- 
Michal Luczaj <mhal@rbox.co>


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

* [PATCH net v2 1/4] Bluetooth: Improve setsockopt() handling of malformed user input
  2024-11-15 13:21 [PATCH net v2 0/4] net: Fix some callers of copy_from_sockptr() Michal Luczaj
@ 2024-11-15 13:21 ` Michal Luczaj
  2024-11-15 13:46   ` net: Fix some callers of copy_from_sockptr() bluez.test.bot
  2024-11-15 13:21 ` [PATCH net v2 2/4] llc: Improve setsockopt() handling of malformed user input Michal Luczaj
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Michal Luczaj @ 2024-11-15 13:21 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
	David S. Miller, Eric Dumazet, Paolo Abeni, Simon Horman,
	David Howells, Marc Dionne
  Cc: Luiz Augusto von Dentz, linux-bluetooth, netdev, linux-afs,
	Jakub Kicinski, Michal Luczaj

The bt_copy_from_sockptr() return value is being misinterpreted by most
users: a non-zero result is mistakenly assumed to represent an error code,
but actually indicates the number of bytes that could not be copied.

Remove bt_copy_from_sockptr() and adapt callers to use
copy_safe_from_sockptr().

For sco_sock_setsockopt() (case BT_CODEC) use copy_struct_from_sockptr() to
scrub parts of uninitialized buffer.

Opportunistically, rename `len` to `optlen` in hci_sock_setsockopt_old()
and hci_sock_setsockopt().

Fixes: 51eda36d33e4 ("Bluetooth: SCO: Fix not validating setsockopt user input")
Fixes: a97de7bff13b ("Bluetooth: RFCOMM: Fix not validating setsockopt user input")
Fixes: 4f3951242ace ("Bluetooth: L2CAP: Fix not validating setsockopt user input")
Fixes: 9e8742cdfc4b ("Bluetooth: ISO: Fix not validating setsockopt user input")
Fixes: b2186061d604 ("Bluetooth: hci_sock: Fix not validating setsockopt user input")
Reviewed-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 include/net/bluetooth/bluetooth.h |  9 ---------
 net/bluetooth/hci_sock.c          | 14 +++++++-------
 net/bluetooth/iso.c               | 10 +++++-----
 net/bluetooth/l2cap_sock.c        | 20 +++++++++++---------
 net/bluetooth/rfcomm/sock.c       |  9 ++++-----
 net/bluetooth/sco.c               | 11 ++++++-----
 6 files changed, 33 insertions(+), 40 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index f66bc85c6411dd5d49eca7756577fea05feaf144..e6760c11f007752ff05792f1de09b70bfb57213c 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -590,15 +590,6 @@ static inline struct sk_buff *bt_skb_sendmmsg(struct sock *sk,
 	return skb;
 }
 
-static inline int bt_copy_from_sockptr(void *dst, size_t dst_size,
-				       sockptr_t src, size_t src_size)
-{
-	if (dst_size > src_size)
-		return -EINVAL;
-
-	return copy_from_sockptr(dst, src, dst_size);
-}
-
 int bt_to_errno(u16 code);
 __u8 bt_status(int err);
 
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 2272e1849ebd894a6b83f665d8fa45610778463c..022b86797acdc56a6e9b85f24f4c98a0247831c9 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -1926,7 +1926,7 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 }
 
 static int hci_sock_setsockopt_old(struct socket *sock, int level, int optname,
-				   sockptr_t optval, unsigned int len)
+				   sockptr_t optval, unsigned int optlen)
 {
 	struct hci_ufilter uf = { .opcode = 0 };
 	struct sock *sk = sock->sk;
@@ -1943,7 +1943,7 @@ static int hci_sock_setsockopt_old(struct socket *sock, int level, int optname,
 
 	switch (optname) {
 	case HCI_DATA_DIR:
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, len);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
@@ -1954,7 +1954,7 @@ static int hci_sock_setsockopt_old(struct socket *sock, int level, int optname,
 		break;
 
 	case HCI_TIME_STAMP:
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, len);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
@@ -1974,7 +1974,7 @@ static int hci_sock_setsockopt_old(struct socket *sock, int level, int optname,
 			uf.event_mask[1] = *((u32 *) f->event_mask + 1);
 		}
 
-		err = bt_copy_from_sockptr(&uf, sizeof(uf), optval, len);
+		err = copy_safe_from_sockptr(&uf, sizeof(uf), optval, optlen);
 		if (err)
 			break;
 
@@ -2005,7 +2005,7 @@ static int hci_sock_setsockopt_old(struct socket *sock, int level, int optname,
 }
 
 static int hci_sock_setsockopt(struct socket *sock, int level, int optname,
-			       sockptr_t optval, unsigned int len)
+			       sockptr_t optval, unsigned int optlen)
 {
 	struct sock *sk = sock->sk;
 	int err = 0;
@@ -2015,7 +2015,7 @@ static int hci_sock_setsockopt(struct socket *sock, int level, int optname,
 
 	if (level == SOL_HCI)
 		return hci_sock_setsockopt_old(sock, level, optname, optval,
-					       len);
+					       optlen);
 
 	if (level != SOL_BLUETOOTH)
 		return -ENOPROTOOPT;
@@ -2035,7 +2035,7 @@ static int hci_sock_setsockopt(struct socket *sock, int level, int optname,
 			goto done;
 		}
 
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, len);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 7a83e400ac77a0a0df41b206643bae6fc031631b..5f278971d7fa25b32b6f70a5fc5a7500db0fdc99 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1503,7 +1503,7 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
@@ -1514,7 +1514,7 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case BT_PKT_STATUS:
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
@@ -1533,7 +1533,7 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		err = bt_copy_from_sockptr(&qos, sizeof(qos), optval, optlen);
+		err = copy_safe_from_sockptr(&qos, sizeof(qos), optval, optlen);
 		if (err)
 			break;
 
@@ -1554,8 +1554,8 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		err = bt_copy_from_sockptr(iso_pi(sk)->base, optlen, optval,
-					   optlen);
+		err = copy_safe_from_sockptr(iso_pi(sk)->base, optlen, optval,
+					     optlen);
 		if (err)
 			break;
 
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ba437c6f6ee591a5624f5fbfbf28f2a80d399372..5ab203b55ab7e2c0682349a6eab9620e3e8a024c 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -755,7 +755,8 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
 		opts.max_tx   = chan->max_tx;
 		opts.txwin_size = chan->tx_win;
 
-		err = bt_copy_from_sockptr(&opts, sizeof(opts), optval, optlen);
+		err = copy_safe_from_sockptr(&opts, sizeof(opts), optval,
+					     optlen);
 		if (err)
 			break;
 
@@ -800,7 +801,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
 		break;
 
 	case L2CAP_LM:
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
@@ -909,7 +910,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 
 		sec.level = BT_SECURITY_LOW;
 
-		err = bt_copy_from_sockptr(&sec, sizeof(sec), optval, optlen);
+		err = copy_safe_from_sockptr(&sec, sizeof(sec), optval, optlen);
 		if (err)
 			break;
 
@@ -956,7 +957,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
@@ -970,7 +971,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case BT_FLUSHABLE:
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
@@ -1004,7 +1005,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 
 		pwr.force_active = BT_POWER_FORCE_ACTIVE_ON;
 
-		err = bt_copy_from_sockptr(&pwr, sizeof(pwr), optval, optlen);
+		err = copy_safe_from_sockptr(&pwr, sizeof(pwr), optval, optlen);
 		if (err)
 			break;
 
@@ -1015,7 +1016,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case BT_CHANNEL_POLICY:
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
@@ -1046,7 +1047,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		err = bt_copy_from_sockptr(&mtu, sizeof(mtu), optval, optlen);
+		err = copy_safe_from_sockptr(&mtu, sizeof(mtu), optval, optlen);
 		if (err)
 			break;
 
@@ -1076,7 +1077,8 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		err = bt_copy_from_sockptr(&mode, sizeof(mode), optval, optlen);
+		err = copy_safe_from_sockptr(&mode, sizeof(mode), optval,
+					     optlen);
 		if (err)
 			break;
 
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index f48250e3f2e103c75d5937e1608e43c123aa3297..1001fb4cc21c0ecc7bcdd3ea9041770ede4f27b8 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -629,10 +629,9 @@ static int rfcomm_sock_setsockopt_old(struct socket *sock, int optname,
 
 	switch (optname) {
 	case RFCOMM_LM:
-		if (bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen)) {
-			err = -EFAULT;
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		if (err)
 			break;
-		}
 
 		if (opt & RFCOMM_LM_FIPS) {
 			err = -EINVAL;
@@ -685,7 +684,7 @@ static int rfcomm_sock_setsockopt(struct socket *sock, int level, int optname,
 
 		sec.level = BT_SECURITY_LOW;
 
-		err = bt_copy_from_sockptr(&sec, sizeof(sec), optval, optlen);
+		err = copy_safe_from_sockptr(&sec, sizeof(sec), optval, optlen);
 		if (err)
 			break;
 
@@ -703,7 +702,7 @@ static int rfcomm_sock_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 1c7252a3686694284b0b1e1101e3d16b90d906c4..700abb639a554521b9b5d46298d5ed926d228470 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -853,7 +853,7 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
@@ -872,8 +872,8 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 
 		voice.setting = sco_pi(sk)->setting;
 
-		err = bt_copy_from_sockptr(&voice, sizeof(voice), optval,
-					   optlen);
+		err = copy_safe_from_sockptr(&voice, sizeof(voice), optval,
+					     optlen);
 		if (err)
 			break;
 
@@ -898,7 +898,7 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case BT_PKT_STATUS:
-		err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
 		if (err)
 			break;
 
@@ -941,7 +941,8 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		err = bt_copy_from_sockptr(buffer, optlen, optval, optlen);
+		err = copy_struct_from_sockptr(buffer, sizeof(buffer), optval,
+					       optlen);
 		if (err) {
 			hci_dev_put(hdev);
 			break;

-- 
2.46.2


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

* [PATCH net v2 2/4] llc: Improve setsockopt() handling of malformed user input
  2024-11-15 13:21 [PATCH net v2 0/4] net: Fix some callers of copy_from_sockptr() Michal Luczaj
  2024-11-15 13:21 ` [PATCH net v2 1/4] Bluetooth: Improve setsockopt() handling of malformed user input Michal Luczaj
@ 2024-11-15 13:21 ` Michal Luczaj
  2024-11-16  0:59   ` David Wei
  2024-11-15 13:21 ` [PATCH net v2 3/4] rxrpc: " Michal Luczaj
  2024-11-15 13:21 ` [PATCH net v2 4/4] net: Comment copy_from_sockptr() explaining its behaviour Michal Luczaj
  3 siblings, 1 reply; 10+ messages in thread
From: Michal Luczaj @ 2024-11-15 13:21 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
	David S. Miller, Eric Dumazet, Paolo Abeni, Simon Horman,
	David Howells, Marc Dionne
  Cc: Luiz Augusto von Dentz, linux-bluetooth, netdev, linux-afs,
	Jakub Kicinski, Michal Luczaj

copy_from_sockptr()'s non-zero result represents the number of bytes that
could not be copied. Turn that into EFAULT.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/llc/af_llc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
index 4eb52add7103b0f83d6fe7318abf1d1af533d254..711c8a7a423f1cf1b03e684a6e23c8eefbab830f 100644
--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -1096,12 +1096,12 @@ static int llc_ui_setsockopt(struct socket *sock, int level, int optname,
 	int rc = -EINVAL;
 
 	lock_sock(sk);
-	if (unlikely(level != SOL_LLC || optlen != sizeof(int)))
+	if (unlikely(level != SOL_LLC || optlen != sizeof(opt)))
 		goto out;
-	rc = copy_from_sockptr(&opt, optval, sizeof(opt));
-	if (rc)
+	if (copy_from_sockptr(&opt, optval, sizeof(opt))) {
+		rc = -EFAULT;
 		goto out;
-	rc = -EINVAL;
+	}
 	switch (optname) {
 	case LLC_OPT_RETRY:
 		if (opt > LLC_OPT_MAX_RETRY)

-- 
2.46.2


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

* [PATCH net v2 3/4] rxrpc: Improve setsockopt() handling of malformed user input
  2024-11-15 13:21 [PATCH net v2 0/4] net: Fix some callers of copy_from_sockptr() Michal Luczaj
  2024-11-15 13:21 ` [PATCH net v2 1/4] Bluetooth: Improve setsockopt() handling of malformed user input Michal Luczaj
  2024-11-15 13:21 ` [PATCH net v2 2/4] llc: Improve setsockopt() handling of malformed user input Michal Luczaj
@ 2024-11-15 13:21 ` Michal Luczaj
  2024-11-15 13:21 ` [PATCH net v2 4/4] net: Comment copy_from_sockptr() explaining its behaviour Michal Luczaj
  3 siblings, 0 replies; 10+ messages in thread
From: Michal Luczaj @ 2024-11-15 13:21 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
	David S. Miller, Eric Dumazet, Paolo Abeni, Simon Horman,
	David Howells, Marc Dionne
  Cc: Luiz Augusto von Dentz, linux-bluetooth, netdev, linux-afs,
	Jakub Kicinski, Michal Luczaj

copy_from_sockptr() doesn't return negative value on error. Instead it's
the number of bytes that could not be copied. Turn that into EFAULT.

Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/rxrpc/af_rxrpc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index f4844683e12039d636253cb06f622468593487eb..dcf64dc148cceb547ffdb1cea8ff53a0633f5c06 100644
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -702,14 +702,14 @@ static int rxrpc_setsockopt(struct socket *sock, int level, int optname,
 
 		case RXRPC_MIN_SECURITY_LEVEL:
 			ret = -EINVAL;
-			if (optlen != sizeof(unsigned int))
+			if (optlen != sizeof(min_sec_level))
 				goto error;
 			ret = -EISCONN;
 			if (rx->sk.sk_state != RXRPC_UNBOUND)
 				goto error;
-			ret = copy_from_sockptr(&min_sec_level, optval,
-				       sizeof(unsigned int));
-			if (ret < 0)
+			ret = -EFAULT;
+			if (copy_from_sockptr(&min_sec_level, optval,
+					      sizeof(min_sec_level)))
 				goto error;
 			ret = -EINVAL;
 			if (min_sec_level > RXRPC_SECURITY_MAX)

-- 
2.46.2


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

* [PATCH net v2 4/4] net: Comment copy_from_sockptr() explaining its behaviour
  2024-11-15 13:21 [PATCH net v2 0/4] net: Fix some callers of copy_from_sockptr() Michal Luczaj
                   ` (2 preceding siblings ...)
  2024-11-15 13:21 ` [PATCH net v2 3/4] rxrpc: " Michal Luczaj
@ 2024-11-15 13:21 ` Michal Luczaj
  3 siblings, 0 replies; 10+ messages in thread
From: Michal Luczaj @ 2024-11-15 13:21 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
	David S. Miller, Eric Dumazet, Paolo Abeni, Simon Horman,
	David Howells, Marc Dionne
  Cc: Luiz Augusto von Dentz, linux-bluetooth, netdev, linux-afs,
	Jakub Kicinski, Michal Luczaj

copy_from_sockptr() has a history of misuse. Add a comment explaining that
the function follows API of copy_from_user(), i.e. returns 0 for success,
or number of bytes not copied on error.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 include/linux/sockptr.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h
index 195debe2b1dbc5abf768aa806eb6c73b99421e27..3e6c8e9d67aef66e8ac5a4e474c278ac08244163 100644
--- a/include/linux/sockptr.h
+++ b/include/linux/sockptr.h
@@ -53,6 +53,8 @@ static inline int copy_from_sockptr_offset(void *dst, sockptr_t src,
 /* Deprecated.
  * This is unsafe, unless caller checked user provided optlen.
  * Prefer copy_safe_from_sockptr() instead.
+ *
+ * Returns 0 for success, or number of bytes not copied on error.
  */
 static inline int copy_from_sockptr(void *dst, sockptr_t src, size_t size)
 {

-- 
2.46.2


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

* RE: net: Fix some callers of copy_from_sockptr()
  2024-11-15 13:21 ` [PATCH net v2 1/4] Bluetooth: Improve setsockopt() handling of malformed user input Michal Luczaj
@ 2024-11-15 13:46   ` bluez.test.bot
  0 siblings, 0 replies; 10+ messages in thread
From: bluez.test.bot @ 2024-11-15 13:46 UTC (permalink / raw)
  To: linux-bluetooth, mhal

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

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=909980

---Test result---

Test Summary:
CheckPatch                    PENDING   0.20 seconds
GitLint                       PENDING   0.18 seconds
SubjectPrefix                 FAIL      0.68 seconds
BuildKernel                   PASS      25.03 seconds
CheckAllWarning               PASS      28.08 seconds
CheckSparse                   WARNING   32.00 seconds
BuildKernel32                 PASS      25.33 seconds
TestRunnerSetup               PASS      452.91 seconds
TestRunner_l2cap-tester       PASS      21.30 seconds
TestRunner_iso-tester         FAIL      33.60 seconds
TestRunner_bnep-tester        PASS      5.04 seconds
TestRunner_mgmt-tester        PASS      120.99 seconds
TestRunner_rfcomm-tester      PASS      7.93 seconds
TestRunner_sco-tester         PASS      11.58 seconds
TestRunner_ioctl-tester       PASS      8.54 seconds
TestRunner_mesh-tester        PASS      6.31 seconds
TestRunner_smp-tester         PASS      7.28 seconds
TestRunner_userchan-tester    PASS      5.42 seconds
IncrementalBuild              PENDING   0.40 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
Total: 124, Passed: 120 (96.8%), Failed: 0, Not Run: 4
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth


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

* Re: [PATCH net v2 2/4] llc: Improve setsockopt() handling of malformed user input
  2024-11-15 13:21 ` [PATCH net v2 2/4] llc: Improve setsockopt() handling of malformed user input Michal Luczaj
@ 2024-11-16  0:59   ` David Wei
  2024-11-16  9:24     ` Michal Luczaj
  0 siblings, 1 reply; 10+ messages in thread
From: David Wei @ 2024-11-16  0:59 UTC (permalink / raw)
  To: Michal Luczaj, Marcel Holtmann, Johan Hedberg,
	Luiz Augusto von Dentz, David S. Miller, Eric Dumazet,
	Paolo Abeni, Simon Horman, David Howells, Marc Dionne
  Cc: Luiz Augusto von Dentz, linux-bluetooth, netdev, linux-afs,
	Jakub Kicinski

On 2024-11-15 05:21, Michal Luczaj wrote:
> copy_from_sockptr()'s non-zero result represents the number of bytes that
> could not be copied. Turn that into EFAULT.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  net/llc/af_llc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> index 4eb52add7103b0f83d6fe7318abf1d1af533d254..711c8a7a423f1cf1b03e684a6e23c8eefbab830f 100644
> --- a/net/llc/af_llc.c
> +++ b/net/llc/af_llc.c
> @@ -1096,12 +1096,12 @@ static int llc_ui_setsockopt(struct socket *sock, int level, int optname,
>  	int rc = -EINVAL;
>  
>  	lock_sock(sk);
> -	if (unlikely(level != SOL_LLC || optlen != sizeof(int)))
> +	if (unlikely(level != SOL_LLC || optlen != sizeof(opt)))
>  		goto out;
> -	rc = copy_from_sockptr(&opt, optval, sizeof(opt));
> -	if (rc)
> +	if (copy_from_sockptr(&opt, optval, sizeof(opt))) {
> +		rc = -EFAULT;
>  		goto out;
> -	rc = -EINVAL;
> +	}
>  	switch (optname) {
>  	case LLC_OPT_RETRY:
>  		if (opt > LLC_OPT_MAX_RETRY)
> 

Can copy_from_sockptr() be deprecated here in favour of
copy_safe_from_sockptr()?

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

* Re: [PATCH net v2 2/4] llc: Improve setsockopt() handling of malformed user input
  2024-11-16  0:59   ` David Wei
@ 2024-11-16  9:24     ` Michal Luczaj
  0 siblings, 0 replies; 10+ messages in thread
From: Michal Luczaj @ 2024-11-16  9:24 UTC (permalink / raw)
  To: David Wei, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
	David S. Miller, Eric Dumazet, Paolo Abeni, Simon Horman,
	David Howells, Marc Dionne
  Cc: Luiz Augusto von Dentz, linux-bluetooth, netdev, linux-afs,
	Jakub Kicinski

On 11/16/24 01:59, David Wei wrote:
> On 2024-11-15 05:21, Michal Luczaj wrote:
>> copy_from_sockptr()'s non-zero result represents the number of bytes that
>> could not be copied. Turn that into EFAULT.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>>  net/llc/af_llc.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
>> index 4eb52add7103b0f83d6fe7318abf1d1af533d254..711c8a7a423f1cf1b03e684a6e23c8eefbab830f 100644
>> --- a/net/llc/af_llc.c
>> +++ b/net/llc/af_llc.c
>> @@ -1096,12 +1096,12 @@ static int llc_ui_setsockopt(struct socket *sock, int level, int optname,
>>  	int rc = -EINVAL;
>>  
>>  	lock_sock(sk);
>> -	if (unlikely(level != SOL_LLC || optlen != sizeof(int)))
>> +	if (unlikely(level != SOL_LLC || optlen != sizeof(opt)))
>>  		goto out;
>> -	rc = copy_from_sockptr(&opt, optval, sizeof(opt));
>> -	if (rc)
>> +	if (copy_from_sockptr(&opt, optval, sizeof(opt))) {
>> +		rc = -EFAULT;
>>  		goto out;
>> -	rc = -EINVAL;
>> +	}
>>  	switch (optname) {
>>  	case LLC_OPT_RETRY:
>>  		if (opt > LLC_OPT_MAX_RETRY)
>>
> 
> Can copy_from_sockptr() be deprecated here in favour of
> copy_safe_from_sockptr()?

Yeah, good point. I'll wait a bit and send v3.

Thanks!


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

* RE: net: Fix some callers of copy_from_sockptr()
  2024-11-19 13:31 [PATCH net v3 1/4] Bluetooth: Improve setsockopt() handling of malformed user input Michal Luczaj
@ 2024-11-19 13:51 ` bluez.test.bot
  0 siblings, 0 replies; 10+ messages in thread
From: bluez.test.bot @ 2024-11-19 13:51 UTC (permalink / raw)
  To: linux-bluetooth, mhal

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

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=910950

---Test result---

Test Summary:
CheckPatch                    PENDING   0.32 seconds
GitLint                       PENDING   0.22 seconds
SubjectPrefix                 FAIL      0.64 seconds
BuildKernel                   PASS      25.64 seconds
CheckAllWarning               PASS      28.00 seconds
CheckSparse                   WARNING   31.47 seconds
BuildKernel32                 PASS      24.81 seconds
TestRunnerSetup               PASS      441.63 seconds
TestRunner_l2cap-tester       PASS      20.75 seconds
TestRunner_iso-tester         FAIL      28.38 seconds
TestRunner_bnep-tester        PASS      4.88 seconds
TestRunner_mgmt-tester        PASS      117.97 seconds
TestRunner_rfcomm-tester      PASS      7.64 seconds
TestRunner_sco-tester         PASS      11.41 seconds
TestRunner_ioctl-tester       PASS      8.36 seconds
TestRunner_mesh-tester        PASS      7.11 seconds
TestRunner_smp-tester         PASS      7.07 seconds
TestRunner_userchan-tester    PASS      5.13 seconds
IncrementalBuild              PENDING   0.49 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
Total: 124, Passed: 120 (96.8%), Failed: 0, Not Run: 4
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth


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

end of thread, other threads:[~2024-11-19 13:51 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-15 13:21 [PATCH net v2 0/4] net: Fix some callers of copy_from_sockptr() Michal Luczaj
2024-11-15 13:21 ` [PATCH net v2 1/4] Bluetooth: Improve setsockopt() handling of malformed user input Michal Luczaj
2024-11-15 13:46   ` net: Fix some callers of copy_from_sockptr() bluez.test.bot
2024-11-15 13:21 ` [PATCH net v2 2/4] llc: Improve setsockopt() handling of malformed user input Michal Luczaj
2024-11-16  0:59   ` David Wei
2024-11-16  9:24     ` Michal Luczaj
2024-11-15 13:21 ` [PATCH net v2 3/4] rxrpc: " Michal Luczaj
2024-11-15 13:21 ` [PATCH net v2 4/4] net: Comment copy_from_sockptr() explaining its behaviour Michal Luczaj
  -- strict thread matches above, loose matches on Subject: below --
2024-11-19 13:31 [PATCH net v3 1/4] Bluetooth: Improve setsockopt() handling of malformed user input Michal Luczaj
2024-11-19 13:51 ` net: Fix some callers of copy_from_sockptr() bluez.test.bot
2024-11-14 23:27 [PATCH net 1/4] bluetooth: Improve setsockopt() handling of malformed user input Michal Luczaj
2024-11-15  1:20 ` net: Fix some callers of copy_from_sockptr() bluez.test.bot

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).