wifi: nl80211: drop impossible negative band check
PATCH 1/1 wifi: nl80211: drop impossible negative band check
band is derived from nla_type() of a nested netlink attribute, which is a masked u16 value and therefore cannot be negative. Drop the dead "band < 0" checks and keep the upper bound validation.
No functional change intended.
--- net/wireless/nl80211.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
net/wireless/nl80211.c +2 -2
@@ -5759,7 +5759,7 @@ static int nl80211_parse_tx_bitrate_mask(struct genl_info *info,enum nl80211_band band =nla_type (tx_rates);int err; -if (band <0 || band >=NUM_NL80211_BANDS ) +if (band >=NUM_NL80211_BANDS )return -EINVAL ; sband = rdev->wiphy.bands[band];if (sband ==NULL )
@@ -10536,7 +10536,7 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info) tmp) {enum nl80211_band band =nla_type (attr); -if (band <0 || band >=NUM_NL80211_BANDS ) { +if (band >=NUM_NL80211_BANDS ) { err = -EINVAL ; goto out_free; }
Signature · 1 line
2.43.0
Johannes Berg
maintainerQuoted 3 lines · Sun Jian
band is derived from
nla_type()of a nested netlink attribute, which is a masked u16 value and therefore cannot be negative. Drop the dead "band < 0" checks and keep the upper bound validation.
I've seen this before, but I'm not really convinced it is entirely correct. C says:
All enumerations have an underlying type. The underlying type can be explicitly specified using an enum type specifier and is its fixed underlying type. If it is not explicitly specified, the underlying type is the enumeration’s compatible type, which is either char or a standard or extended signed or unsigned integer type.
It would thus _seem_ to be possible for an enum to generally be a signed type, and therefore a 'signed short', and therefore an nla_type() that's a u16 could end up with a negative value...
Am I wrong?
johannes
Quoted 6 lines · Johannes Berg
Quoted 3 lines · Sun Jian
band is derived from
nla_type()of a nested netlink attribute, which isAgreed — in general the enum underlying type can be signed. a masked u16 value and therefore cannot be negative. Drop the dead
"band < 0" checks and keep the upper bound validation.
Quoted 9 lines
I've seen this before, but I'm not really convinced it is entirely correct. C says:
All enumerations have an underlying type. The underlying type can be explicitly specified using an enum type specifier and is its fixed underlying type. If it is not explicitly specified, the underlying type is the enumeration’s compatible type, which is either char or a standard or extended signed or unsigned integer type.
Agreed — in general the enum underlying type can be signed.
Quoted 3 lines
It would thus _seem_ to be possible for an enum to generally be a signed type, and therefore a 'signed short', and therefore an
nla_type()that's a u16 could end up with a negative value...
The key detail here is that band isn't assigned the raw __u16 nla->nla_type, but nla_type().
And nla_type() is effectively:nla->nla_type & NLA_TYPE_MASK
and NLA_TYPE_MASK clears the two high flag bits:NLA_F_NESTED (1 << 15)NLA_F_NET_BYTEORDER (1 << 14)
So the result is restricted to the low 14 bits, i.e. 0..0x3fff.
With that restriction, even if enum nl80211_band ended up with a signed 16-bit underlying type, the sign bit (bit 15) can never be set by nla_type(), so the value cannot become negative.
Quoted 2 lines
Am I wrong?
I think the "enum may be signed" concern is valid in general, but for this particular assignment the masking guarantees the value is always in a non-negative range.
Thanks,
Sun Jian
Johannes Berg
maintainerQuoted 10 lines · sun jian
Quoted 9 lines · Johannes Berg
Quoted 3 lines · Sun Jian
band is derived from
nla_type()of a nested netlink attribute, which isAgreed — in general the enum underlying type can be signed. a masked u16 value and therefore cannot be negative. Drop the dead
"band < 0" checks and keep the upper bound validation.I've seen this before, but I'm not really convinced it is entirely correct. C says:
Quoted 8 lines
Quoted 6 lines
All enumerations have an underlying type. The underlying type can be explicitly specified using an enum type specifier and is its fixed underlying type. If it is not explicitly specified, the underlying type is the enumeration’s compatible type, which is either char or a standard or extended signed or unsigned integer type.
Agreed — in general the enum underlying type can be signed.
But nothing says it cannot be "signed char".
Quoted 3 lines
Quoted 3 lines
It would thus _seem_ to be possible for an enum to generally be a signed type, and therefore a 'signed short', and therefore an
nla_type()that's a u16 could end up with a negative value...
I was just using 'signed short' as an example, but your argument:
Quoted 10 lines
The key detail here is that band isn't assigned the raw
__u16nla->nla_type, butnla_type().And
nla_type()is effectively:nla->nla_type&NLA_TYPE_MASKand
NLA_TYPE_MASKclears the two high flag bits:NLA_F_NESTED(1 << 15)NLA_F_NET_BYTEORDER(1 << 14)
Quoted 5 lines
So the result is restricted to the low 14 bits, i.e. 0..0x3fff.
With that restriction, even if enum
nl80211_bandended up with a signed 16-bit underlying type, the sign bit (bit 15) can never be set bynla_type(), so the value cannot become negative.
applies _only_ to signed short, not to signed char?
Now we can argue a "sane compiler" won't do that, and we can also argue that "gcc and clang are sane compilers", although sometimes I definitely have doubts about the latter ;-)
johannes
Quoted 2 lines
applies _only_ to signed short, not to signed char?
Fair point, thanks. I'll drop this patch.
Regards,
Sun Jian
Johannes Berg
maintainerQuoted 3 lines · sun jian
Quoted 2 lines
applies _only_ to signed short, not to signed char?
Fair point, thanks. I'll drop this patch.
I've thought the better way to address that warning would be to simply use 'int' instead of the enum there, but I forgot where the warning even appears. I don't think it's generally with gcc/clang, is it?
johannes
Quoted 10 lines · Johannes Berg
Quoted 3 lines · sun jian
Quoted 2 lines
applies _only_ to signed short, not to signed char?
Fair point, thanks. I'll drop this patch.
I've thought the better way to address that warning would be to simply use 'int' instead of the enum there, but I forgot where the warning even appears. I don't think it's generally with gcc/clang, is it?
Right, I only saw it from sparse, not from gcc/clang.
It was reported for net/wireless/nl80211.c at the checks around lines ~5762 and ~10539 in my tree (both are "band < 0 || band >= NUM_NL80211_BANDS" with band coming from nla_type()).
Thanks,
Sun Jian
Quoted 12 lines · sun jian
Quoted 10 lines · Johannes Berg
Quoted 3 lines · sun jian
Quoted 2 lines
applies _only_ to signed short, not to signed char?
Fair point, thanks. I'll drop this patch.
I've thought the better way to address that warning would be to simply use 'int' instead of the enum there, but I forgot where the warning even appears. I don't think it's generally with gcc/clang, is it?
Quoted 5 lines
Right, I only saw it from sparse, not from gcc/clang.
It was reported for
net/wireless/nl80211.cat the checks around lines ~5762 and ~10539 in my tree (both are "band < 0 || band >=NUM_NL80211_BANDS" with band coming fromnla_type()).
For future reference, if you are fixing a warning, then your commit text should include that information. That way someone else who has that warning can search the archives for that warning and then find your fix.
/jeff
Quoted 5 lines · Jeff Johnson
For future reference, if you are fixing a warning, then your commit text should include that information. That way someone else who has that warning can search the archives for that warning and then find your fix.
Ack, thanks, I will include the exact warning text in future warning-fix commits.
sun jian