| Message ID | 20230526-l2cap-16bit-v1-1-2cfb83dd28ee@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: L2CAP: don't check for out-of-bounds value | expand |
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
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.
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
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; }
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(-)