* [PATCH] Bluetooth: L2CAP: don't check for out-of-bounds value
@ 2023-05-26 14:16 Simon Horman
2023-05-26 14:34 ` Dan Carpenter
2023-05-26 14:57 ` bluez.test.bot
0 siblings, 2 replies; 5+ messages in thread
From: Simon Horman @ 2023-05-26 14:16 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz
Cc: Dan Carpenter, linux-bluetooth, Simon Horman
In l2cap_connect(), scid is an unsigned 16bit variable. Thus, it's
maximum value is L2CAP_CID_DYN_END (0xffff) and there is no need
to check for this value being exceeded.
Flagged by Smatch as:
.../l2cap_core.c:4165 l2cap_connect() warn: impossible condition '(scid > 65535) => (0-u16max > u16max)'
Signed-off-by: Simon Horman <horms@kernel.org>
---
net/bluetooth/l2cap_core.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ae397c6819d9..a5d85a5f5930 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4161,8 +4161,12 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
result = L2CAP_CR_NO_MEM;
- /* Check for valid dynamic CID range (as per Erratum 3253) */
- if (scid < L2CAP_CID_DYN_START || scid > L2CAP_CID_DYN_END) {
+ /* Check for valid dynamic CID range (as per Erratum 3253).
+ * As scid is an unsigned 16bit variable it's maximum
+ * value is L2CAP_CID_DYN_END (0xffff): there is no need to check
+ * if scid exceeds that value here.
+ */
+ if (scid < L2CAP_CID_DYN_START) {
result = L2CAP_CR_INVALID_SCID;
goto response;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: L2CAP: don't check for out-of-bounds value
2023-05-26 14:16 [PATCH] Bluetooth: L2CAP: don't check for out-of-bounds value Simon Horman
@ 2023-05-26 14:34 ` Dan Carpenter
2023-05-26 15:19 ` Simon Horman
2023-05-26 14:57 ` bluez.test.bot
1 sibling, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2023-05-26 14:34 UTC (permalink / raw)
To: Simon Horman
Cc: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, linux-bluetooth
On Fri, May 26, 2023 at 04:16:54PM +0200, Simon Horman wrote:
> @@ -4161,8 +4161,12 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
>
> result = L2CAP_CR_NO_MEM;
>
> - /* Check for valid dynamic CID range (as per Erratum 3253) */
> - if (scid < L2CAP_CID_DYN_START || scid > L2CAP_CID_DYN_END) {
> + /* Check for valid dynamic CID range (as per Erratum 3253).
> + * As scid is an unsigned 16bit variable it's maximum
> + * value is L2CAP_CID_DYN_END (0xffff): there is no need to check
> + * if scid exceeds that value here.
> + */
> + if (scid < L2CAP_CID_DYN_START) {
This is a false positive. To me the warning looks reasonable. But one
way we could silence it would be to keep a list of macros where the
check is impossible but we still want to have it.
I could create something where we do:
echo "L2CAP_CID_DYN_END" >> smatch_data/kernel.allowed_impossible_limits
I'd do the same for unsigned comparisons with zero like:
if (dpmcp_dev->obj_desc.ver_major < DPMCP_MIN_VER_MAJOR ||
(dpmcp_dev->obj_desc.ver_major == DPMCP_MIN_VER_MAJOR &&
dpmcp_dev->obj_desc.ver_minor < DPMCP_MIN_VER_MINOR)) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
dev_err(&dpmcp_dev->dev,
"ERROR: Version %d.%d of DPMCP not supported.\n",
echo "DPMCP_MIN_VER_MINOR" >> smatch_data/kernel.allowed_impossible_limits
I can do that on Monday if you want. Other static checkers might
complain still though.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: Bluetooth: L2CAP: don't check for out-of-bounds value
2023-05-26 14:16 [PATCH] Bluetooth: L2CAP: don't check for out-of-bounds value Simon Horman
2023-05-26 14:34 ` Dan Carpenter
@ 2023-05-26 14:57 ` bluez.test.bot
1 sibling, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2023-05-26 14:57 UTC (permalink / raw)
To: linux-bluetooth, horms
[-- Attachment #1: Type: text/plain, Size: 2070 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=751393
---Test result---
Test Summary:
CheckPatch PASS 0.73 seconds
GitLint FAIL 0.59 seconds
SubjectPrefix PASS 0.13 seconds
BuildKernel PASS 32.79 seconds
CheckAllWarning PASS 35.87 seconds
CheckSparse PASS 40.68 seconds
CheckSmatch PASS 110.56 seconds
BuildKernel32 PASS 31.81 seconds
TestRunnerSetup PASS 451.39 seconds
TestRunner_l2cap-tester PASS 17.35 seconds
TestRunner_iso-tester PASS 22.55 seconds
TestRunner_bnep-tester PASS 5.84 seconds
TestRunner_mgmt-tester PASS 117.28 seconds
TestRunner_rfcomm-tester PASS 9.20 seconds
TestRunner_sco-tester PASS 8.50 seconds
TestRunner_ioctl-tester PASS 9.89 seconds
TestRunner_mesh-tester PASS 7.29 seconds
TestRunner_smp-tester PASS 8.38 seconds
TestRunner_userchan-tester PASS 6.04 seconds
IncrementalBuild PASS 30.04 seconds
Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: L2CAP: don't check for out-of-bounds value
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
9: B1 Line exceeds max length (106>80): " .../l2cap_core.c:4165 l2cap_connect() warn: impossible condition '(scid > 65535) => (0-u16max > u16max)'"
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: L2CAP: don't check for out-of-bounds value
2023-05-26 14:34 ` Dan Carpenter
@ 2023-05-26 15:19 ` Simon Horman
2023-05-30 6:33 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2023-05-26 15:19 UTC (permalink / raw)
To: Dan Carpenter
Cc: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, linux-bluetooth
On Fri, May 26, 2023 at 05:34:30PM +0300, Dan Carpenter wrote:
> On Fri, May 26, 2023 at 04:16:54PM +0200, Simon Horman wrote:
> > @@ -4161,8 +4161,12 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
> >
> > result = L2CAP_CR_NO_MEM;
> >
> > - /* Check for valid dynamic CID range (as per Erratum 3253) */
> > - if (scid < L2CAP_CID_DYN_START || scid > L2CAP_CID_DYN_END) {
> > + /* Check for valid dynamic CID range (as per Erratum 3253).
> > + * As scid is an unsigned 16bit variable it's maximum
> > + * value is L2CAP_CID_DYN_END (0xffff): there is no need to check
> > + * if scid exceeds that value here.
> > + */
> > + if (scid < L2CAP_CID_DYN_START) {
>
> This is a false positive. To me the warning looks reasonable. But one
> way we could silence it would be to keep a list of macros where the
> check is impossible but we still want to have it.
Hi Dan,
I do agree that the existing code is harmless.
Is this why you feel it is a false positive?
> I could create something where we do:
>
> echo "L2CAP_CID_DYN_END" >> smatch_data/kernel.allowed_impossible_limits
>
> I'd do the same for unsigned comparisons with zero like:
>
>
> if (dpmcp_dev->obj_desc.ver_major < DPMCP_MIN_VER_MAJOR ||
> (dpmcp_dev->obj_desc.ver_major == DPMCP_MIN_VER_MAJOR &&
> dpmcp_dev->obj_desc.ver_minor < DPMCP_MIN_VER_MINOR)) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> dev_err(&dpmcp_dev->dev,
> "ERROR: Version %d.%d of DPMCP not supported.\n",
>
> echo "DPMCP_MIN_VER_MINOR" >> smatch_data/kernel.allowed_impossible_limits
FWIIW, I've noticed problems with comparisons to enums. Which, f.e., may in
practice are unsigned values of a particular width for a given build.
But in theory could be any type.
Perhaps the system you propose above would be useful for silencing
warnings about such problems? They seem to be a subset of the problem
at hand.
> I can do that on Monday if you want. Other static checkers might
> complain still though.
No rush from my side.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: L2CAP: don't check for out-of-bounds value
2023-05-26 15:19 ` Simon Horman
@ 2023-05-30 6:33 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2023-05-30 6:33 UTC (permalink / raw)
To: Simon Horman
Cc: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, linux-bluetooth
On Fri, May 26, 2023 at 05:19:52PM +0200, Simon Horman wrote:
> On Fri, May 26, 2023 at 05:34:30PM +0300, Dan Carpenter wrote:
> > On Fri, May 26, 2023 at 04:16:54PM +0200, Simon Horman wrote:
> > > @@ -4161,8 +4161,12 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
> > >
> > > result = L2CAP_CR_NO_MEM;
> > >
> > > - /* Check for valid dynamic CID range (as per Erratum 3253) */
> > > - if (scid < L2CAP_CID_DYN_START || scid > L2CAP_CID_DYN_END) {
> > > + /* Check for valid dynamic CID range (as per Erratum 3253).
> > > + * As scid is an unsigned 16bit variable it's maximum
> > > + * value is L2CAP_CID_DYN_END (0xffff): there is no need to check
> > > + * if scid exceeds that value here.
> > > + */
> > > + if (scid < L2CAP_CID_DYN_START) {
> >
> > This is a false positive. To me the warning looks reasonable. But one
> > way we could silence it would be to keep a list of macros where the
> > check is impossible but we still want to have it.
>
> Hi Dan,
>
> I do agree that the existing code is harmless.
> Is this why you feel it is a false positive?
>
Actually I was thinking of something else, but the other reason why this
is harmless is because it's part of a "clamp both upper and lower
bounds" condition. Linus doesn't like these warnings because it's clear
to a human reader what the intent is. I re-wrote this code last week to
avoid this kind of warning. I will push that so now it won't warn. I
still need to tweak the re-written code a bit.
> > I could create something where we do:
> >
> > echo "L2CAP_CID_DYN_END" >> smatch_data/kernel.allowed_impossible_limits
> >
> > I'd do the same for unsigned comparisons with zero like:
> >
> >
> > if (dpmcp_dev->obj_desc.ver_major < DPMCP_MIN_VER_MAJOR ||
> > (dpmcp_dev->obj_desc.ver_major == DPMCP_MIN_VER_MAJOR &&
> > dpmcp_dev->obj_desc.ver_minor < DPMCP_MIN_VER_MINOR)) {
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > dev_err(&dpmcp_dev->dev,
> > "ERROR: Version %d.%d of DPMCP not supported.\n",
> >
> > echo "DPMCP_MIN_VER_MINOR" >> smatch_data/kernel.allowed_impossible_limits
>
> FWIIW, I've noticed problems with comparisons to enums. Which, f.e., may in
> practice are unsigned values of a particular width for a given build.
> But in theory could be any type.
Enum types are undefined in C but I think Sparse (and thus Smatch)
follow the same rules as GCC basically so they should catch these bugs.
int x;
if (x < ENUM_ZERO) <-- x is negative but type promoted to uint
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-05-30 6:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-26 14:16 [PATCH] Bluetooth: L2CAP: don't check for out-of-bounds value Simon Horman
2023-05-26 14:34 ` Dan Carpenter
2023-05-26 15:19 ` Simon Horman
2023-05-30 6:33 ` Dan Carpenter
2023-05-26 14:57 ` 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).