Bluetooth: L2CAP: don't check for out-of-bounds value

Commit Message

Simon Horman May 26, 2023, 2:16 p.m. UTC

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 <[e-mail]>
---
 net/bluetooth/l2cap_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Dan Carpenter May 26, 2023, 2:34 p.m. UTC | #1

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

[e-mail] May 26, 2023, 2:57 p.m. UTC | #2

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

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=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

Simon Horman May 26, 2023, 3:19 p.m. UTC | #3

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.

Dan Carpenter May 30, 2023, 6:33 a.m. UTC | #4

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

Patch

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;
 	}