| Message ID | 20241115-sockptr-copy-fixes-v1-1-d183c87fcbd5@rbox.co (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: Fix some callers of copy_from_sockptr() | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/SubjectPrefix | fail | "Bluetooth: " prefix is not specified in the subject |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | warning | CheckSparse WARNING net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:147:35: warning: array of flexible structures |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | fail | TestRunner_iso-tester: WARNING: possible circular locking dependency detected |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
On 2024-11-14 15:27, Michal Luczaj wrote: > 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") > 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/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; > - } This will return a positive integer if copy_safe_from_sockptr() fails. Shouldn't this be: err = -EFAULT; if (copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen)) break;
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
Hi David, On Thu, Nov 14, 2024 at 7:42 PM David Wei <dw@davidwei.uk> wrote: > > On 2024-11-14 15:27, Michal Luczaj wrote: > > 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") > > 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/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; > > - } > > This will return a positive integer if copy_safe_from_sockptr() fails. What are you talking about copy_safe_from_sockptr never returns a positive value: * Returns: * * -EINVAL: @optlen < @ksize * * -EFAULT: access to userspace failed. * * 0 : @ksize bytes were copied > Shouldn't this be: > > err = -EFAULT; > if (copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen)) > break;
Hi Michal, On Thu, Nov 14, 2024 at 6:33 PM Michal Luczaj <mhal@rbox.co> wrote: > > 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") > Signed-off-by: Michal Luczaj <mhal@rbox.co> Reviewed-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > 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 >
On 2024-11-14 18:15, Luiz Augusto von Dentz wrote: > Hi David, > > On Thu, Nov 14, 2024 at 7:42 PM David Wei <dw@davidwei.uk> wrote: >> >> On 2024-11-14 15:27, Michal Luczaj wrote: >>> 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") >>> 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/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; >>> - } >> >> This will return a positive integer if copy_safe_from_sockptr() fails. > > What are you talking about copy_safe_from_sockptr never returns a > positive value: > > * Returns: > * * -EINVAL: @optlen < @ksize > * * -EFAULT: access to userspace failed. > * * 0 : @ksize bytes were copied Isn't this what this series is about? copy_from_sockptr() returns 0 on success, or a positive integer for number of bytes NOT copied on error. Patch 4 even updates the docs for copy_from_sockptr(). copy_safe_from_sockptr() -> copy_from_sockptr() -> copy_from_sockptr_offset() -> memcpy() for kernel to kernel OR -> copy_from_user() otherwise And copy_from_user() follows the same 0 for success or N > 0 for failure. It does not EFAULT on its own AFAIK. The docs for copy_safe_from_sockptr() that you've linked contains the exact misunderstanding that Michal is correcting. > >> Shouldn't this be: >> >> err = -EFAULT; >> if (copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen)) >> break; > > >
Hi David, On Thu, Nov 14, 2024 at 9:30 PM David Wei <dw@davidwei.uk> wrote: > > On 2024-11-14 18:15, Luiz Augusto von Dentz wrote: > > Hi David, > > > > On Thu, Nov 14, 2024 at 7:42 PM David Wei <dw@davidwei.uk> wrote: > >> > >> On 2024-11-14 15:27, Michal Luczaj wrote: > >>> 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") > >>> 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/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; > >>> - } > >> > >> This will return a positive integer if copy_safe_from_sockptr() fails. > > > > What are you talking about copy_safe_from_sockptr never returns a > > positive value: > > > > * Returns: > > * * -EINVAL: @optlen < @ksize > > * * -EFAULT: access to userspace failed. > > * * 0 : @ksize bytes were copied > > Isn't this what this series is about? copy_from_sockptr() returns 0 on > success, or a positive integer for number of bytes NOT copied on error. > Patch 4 even updates the docs for copy_from_sockptr(). > > copy_safe_from_sockptr() > -> copy_from_sockptr() > -> copy_from_sockptr_offset() > -> memcpy() for kernel to kernel OR > -> copy_from_user() otherwise Well except the safe version does check what would otherwise cause a positive return by the likes of copy_from_user and returns -EINVAL instead, otherwise the documentation of copy_safe_from_sockptr is just wrong and shall state that it could return positive as well but I guess that would just make it as inconvenient so we might as well detect when a positive value would be returned just return -EFAULT instead. > And copy_from_user() follows the same 0 for success or N > 0 for > failure. It does not EFAULT on its own AFAIK. > > The docs for copy_safe_from_sockptr() that you've linked contains the > exact misunderstanding that Michal is correcting. > > > > >> Shouldn't this be: > >> > >> err = -EFAULT; > >> if (copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen)) > >> break; > > > > > >
On 2024-11-14 18:50, Luiz Augusto von Dentz wrote: > Hi David, > > On Thu, Nov 14, 2024 at 9:30 PM David Wei <dw@davidwei.uk> wrote: >> >> On 2024-11-14 18:15, Luiz Augusto von Dentz wrote: >>> Hi David, >>> >>> On Thu, Nov 14, 2024 at 7:42 PM David Wei <dw@davidwei.uk> wrote: >>>> >>>> On 2024-11-14 15:27, Michal Luczaj wrote: >>>>> 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") >>>>> 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/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; >>>>> - } >>>> >>>> This will return a positive integer if copy_safe_from_sockptr() fails. >>> >>> What are you talking about copy_safe_from_sockptr never returns a >>> positive value: >>> >>> * Returns: >>> * * -EINVAL: @optlen < @ksize >>> * * -EFAULT: access to userspace failed. >>> * * 0 : @ksize bytes were copied >> >> Isn't this what this series is about? copy_from_sockptr() returns 0 on >> success, or a positive integer for number of bytes NOT copied on error. >> Patch 4 even updates the docs for copy_from_sockptr(). >> >> copy_safe_from_sockptr() >> -> copy_from_sockptr() >> -> copy_from_sockptr_offset() >> -> memcpy() for kernel to kernel OR >> -> copy_from_user() otherwise > > Well except the safe version does check what would otherwise cause a > positive return by the likes of copy_from_user and returns -EINVAL > instead, otherwise the documentation of copy_safe_from_sockptr is just > wrong and shall state that it could return positive as well but I > guess that would just make it as inconvenient so we might as well > detect when a positive value would be returned just return -EFAULT > instead. Yes it checks and returns EINVAL, but not EFAULT which is what my comment on the original patch is about. Most of the calls to bt_copy_from_sockptr() that Michal replaced with copy_safe_from_sockptr() remain incorrect because it is assumed that EFAULT is returned. Only rfcomm_sock_setsockopt_old() was vaguely doing the right thing and the patch changed it back to the incorrect pattern: err = copy_safe_from_sockptr(...); if (err) break; But I do agree that making copy_safe_from_sockptr() do the right thing and EFAULT will be easier and prevent future problems given that copy_from_sockptr() is meant to be deprecated anyhow. > >> And copy_from_user() follows the same 0 for success or N > 0 for >> failure. It does not EFAULT on its own AFAIK. >> >> The docs for copy_safe_from_sockptr() that you've linked contains the >> exact misunderstanding that Michal is correcting. >> >>> >>>> Shouldn't this be: >>>> >>>> err = -EFAULT; >>>> if (copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen)) >>>> break; >>> >>> >>> > > >
On 11/15/24 04:04, David Wei wrote: > On 2024-11-14 18:50, Luiz Augusto von Dentz wrote: >> Hi David, >> On Thu, Nov 14, 2024 at 9:30 PM David Wei <dw@davidwei.uk> wrote: >>> On 2024-11-14 18:15, Luiz Augusto von Dentz wrote: >>>> Hi David, >>>> On Thu, Nov 14, 2024 at 7:42 PM David Wei <dw@davidwei.uk> wrote: >>>>> On 2024-11-14 15:27, Michal Luczaj wrote: >>>>> ... >>>>>> 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; >>>>>> - } >>>>> >>>>> This will return a positive integer if copy_safe_from_sockptr() fails. >>>> >>>> What are you talking about copy_safe_from_sockptr never returns a >>>> positive value: >>>> >>>> * Returns: >>>> * * -EINVAL: @optlen < @ksize >>>> * * -EFAULT: access to userspace failed. >>>> * * 0 : @ksize bytes were copied >>> >>> Isn't this what this series is about? copy_from_sockptr() returns 0 on >>> success, or a positive integer for number of bytes NOT copied on error. >>> Patch 4 even updates the docs for copy_from_sockptr(). >>> >>> copy_safe_from_sockptr() >>> -> copy_from_sockptr() >>> -> copy_from_sockptr_offset() >>> -> memcpy() for kernel to kernel OR >>> -> copy_from_user() otherwise >> >> Well except the safe version does check what would otherwise cause a >> positive return by the likes of copy_from_user and returns -EINVAL >> instead, otherwise the documentation of copy_safe_from_sockptr is just >> wrong and shall state that it could return positive as well but I >> guess that would just make it as inconvenient so we might as well >> detect when a positive value would be returned just return -EFAULT >> instead. > > Yes it checks and returns EINVAL, but not EFAULT which is what my > comment on the original patch is about. Most of the calls to > bt_copy_from_sockptr() that Michal replaced with > copy_safe_from_sockptr() remain incorrect because it is assumed that > EFAULT is returned. Only rfcomm_sock_setsockopt_old() was vaguely doing > the right thing and the patch changed it back to the incorrect pattern: > > err = copy_safe_from_sockptr(...); > if (err) > break; > > But I do agree that making copy_safe_from_sockptr() do the right thing > and EFAULT will be easier and prevent future problems given that > copy_from_sockptr() is meant to be deprecated anyhow. Just to be clear: copy_safe_from_sockptr() was recently fixed to return EFAULT: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=eb94b7bb1010 Sorry, I should have mentioned this series is a follow up to that patch. Thanks, Michal
On 2024-11-15 00:31, Michal Luczaj wrote: > On 11/15/24 04:04, David Wei wrote: >> On 2024-11-14 18:50, Luiz Augusto von Dentz wrote: >>> Hi David, >>> On Thu, Nov 14, 2024 at 9:30 PM David Wei <dw@davidwei.uk> wrote: >>>> On 2024-11-14 18:15, Luiz Augusto von Dentz wrote: >>>>> Hi David, >>>>> On Thu, Nov 14, 2024 at 7:42 PM David Wei <dw@davidwei.uk> wrote: >>>>>> On 2024-11-14 15:27, Michal Luczaj wrote: >>>>>> ... >>>>>>> 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; >>>>>>> - } >>>>>> >>>>>> This will return a positive integer if copy_safe_from_sockptr() fails. >>>>> >>>>> What are you talking about copy_safe_from_sockptr never returns a >>>>> positive value: >>>>> >>>>> * Returns: >>>>> * * -EINVAL: @optlen < @ksize >>>>> * * -EFAULT: access to userspace failed. >>>>> * * 0 : @ksize bytes were copied >>>> >>>> Isn't this what this series is about? copy_from_sockptr() returns 0 on >>>> success, or a positive integer for number of bytes NOT copied on error. >>>> Patch 4 even updates the docs for copy_from_sockptr(). >>>> >>>> copy_safe_from_sockptr() >>>> -> copy_from_sockptr() >>>> -> copy_from_sockptr_offset() >>>> -> memcpy() for kernel to kernel OR >>>> -> copy_from_user() otherwise >>> >>> Well except the safe version does check what would otherwise cause a >>> positive return by the likes of copy_from_user and returns -EINVAL >>> instead, otherwise the documentation of copy_safe_from_sockptr is just >>> wrong and shall state that it could return positive as well but I >>> guess that would just make it as inconvenient so we might as well >>> detect when a positive value would be returned just return -EFAULT >>> instead. >> >> Yes it checks and returns EINVAL, but not EFAULT which is what my >> comment on the original patch is about. Most of the calls to >> bt_copy_from_sockptr() that Michal replaced with >> copy_safe_from_sockptr() remain incorrect because it is assumed that >> EFAULT is returned. Only rfcomm_sock_setsockopt_old() was vaguely doing >> the right thing and the patch changed it back to the incorrect pattern: >> >> err = copy_safe_from_sockptr(...); >> if (err) >> break; >> >> But I do agree that making copy_safe_from_sockptr() do the right thing >> and EFAULT will be easier and prevent future problems given that >> copy_from_sockptr() is meant to be deprecated anyhow. > > Just to be clear: copy_safe_from_sockptr() was recently fixed to return > EFAULT: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=eb94b7bb1010 > Sorry, I should have mentioned this series is a follow up to that patch. I missed that, sorry. In which case this patch looks good. Reviewed-by: David Wei <dw@davidwei.uk> > > Thanks, > Michal >
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;
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") 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(-)