| Message ID | 20141024081534.GA11140@mwanda (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, 2014-10-24 at 11:15 +0300, Dan Carpenter wrote: > The ->parent_tsf[] array holds u8 values. It type promoted to int for > the shift operation so the "<< 24" shift operation can wrap. The cast > needs to be done before the shift instead of after. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > Static checker work. Untested. > > diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c [] > @@ -7819,10 +7819,10 @@ static void ipw_handle_data_packet_monitor(struct ipw_priv *priv, > > /* Zero the flags, we'll add to them as we go */ > ipw_rt->rt_flags = 0; > - ipw_rt->rt_tsf = (u64)(frame->parent_tsf[3] << 24 | > - frame->parent_tsf[2] << 16 | > - frame->parent_tsf[1] << 8 | > - frame->parent_tsf[0]); > + ipw_rt->rt_tsf = (u64)frame->parent_tsf[3] << 24 | > + frame->parent_tsf[2] << 16 | > + frame->parent_tsf[1] << 8 | > + frame->parent_tsf[0]; > > /* Convert signal to DBM */ > ipw_rt->rt_dbmsignal = antsignal; > @@ -8028,10 +8028,10 @@ static void ipw_handle_promiscuous_rx(struct ipw_priv *priv, > > /* Zero the flags, we'll add to them as we go */ > ipw_rt->rt_flags = 0; > - ipw_rt->rt_tsf = (u64)(frame->parent_tsf[3] << 24 | > - frame->parent_tsf[2] << 16 | > - frame->parent_tsf[1] << 8 | > - frame->parent_tsf[0]); > + ipw_rt->rt_tsf = (u64)frame->parent_tsf[3] << 24 | > + frame->parent_tsf[2] << 16 | > + frame->parent_tsf[1] << 8 | > + frame->parent_tsf[0]; > > /* Convert to DBM */ > ipw_rt->rt_dbmsignal = signal; struct ipw_rt_hdr { struct ieee80211_radiotap_header rt_hdr; u64 rt_tsf; /* TSF */ /* XXX */ u8 rt_flags; /* radiotap packet flags * u8 rt_rate; /* rate in 500kb/s */ __le16 rt_channel; /* channel in mhz */ __le16 rt_chbitmask; /* channel bitfield */ s8 rt_dbmsignal; /* signal in dbM, kluged to signed */ s8 rt_dbmnoise; u8 rt_antenna; /* antenna number */ u8 payload[0]; /* payload... */ } __packed; Maybe rt_tsf (which is otherwise unused in this code), should be __le64 so maybe use (u32) ? ipw_rt->rt_txf = cpu_to_le64((u32)(frame->parent_tsf[3] << 24 | frame->parent_tsf[2] << 16 | frame->parent_tsf[1] << 8 | frame->parent_tsf[0])); Al Viro touched this with commit 83f7d57c and added the XXX when he did a bunch of type conversions from u<foo> to __le<foo> Dunno what's right. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 24, 2014 at 02:43:31AM -0700, Joe Perches wrote: > On Fri, 2014-10-24 at 11:15 +0300, Dan Carpenter wrote: > > @@ -8028,10 +8028,10 @@ static void ipw_handle_promiscuous_rx(struct ipw_priv *priv, > > > > /* Zero the flags, we'll add to them as we go */ > > ipw_rt->rt_flags = 0; > > - ipw_rt->rt_tsf = (u64)(frame->parent_tsf[3] << 24 | > > - frame->parent_tsf[2] << 16 | > > - frame->parent_tsf[1] << 8 | > > - frame->parent_tsf[0]); > > + ipw_rt->rt_tsf = (u64)frame->parent_tsf[3] << 24 | > > + frame->parent_tsf[2] << 16 | > > + frame->parent_tsf[1] << 8 | > > + frame->parent_tsf[0]; > > > > /* Convert to DBM */ > > ipw_rt->rt_dbmsignal = signal; > > struct ipw_rt_hdr { > struct ieee80211_radiotap_header rt_hdr; > u64 rt_tsf; /* TSF */ /* XXX */ > u8 rt_flags; /* radiotap packet flags * > u8 rt_rate; /* rate in 500kb/s */ > __le16 rt_channel; /* channel in mhz */ > __le16 rt_chbitmask; /* channel bitfield */ > s8 rt_dbmsignal; /* signal in dbM, kluged to signed */ > s8 rt_dbmnoise; > u8 rt_antenna; /* antenna number */ > u8 payload[0]; /* payload... */ > } __packed; > > Maybe rt_tsf (which is otherwise unused in this code), > should be __le64 so maybe use (u32) ? > > ipw_rt->rt_txf = cpu_to_le64((u32)(frame->parent_tsf[3] << 24 | > frame->parent_tsf[2] << 16 | > frame->parent_tsf[1] << 8 | > frame->parent_tsf[0])); > Hm... It don't think it makes sense to truncate the top bits away by truncating to u32. You may be right though that there is some larger bugs here than just the truncation. Both the "frame" and the "ipw_rt" struct seem to hold little endian values generally so probably ->rt_txf should be an __le64 like you say. Perhaps the maintainers know what should be done? regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-10-27 at 12:52 +0300, Dan Carpenter wrote: > On Fri, Oct 24, 2014 at 02:43:31AM -0700, Joe Perches wrote: > > On Fri, 2014-10-24 at 11:15 +0300, Dan Carpenter wrote: > > > @@ -8028,10 +8028,10 @@ static void ipw_handle_promiscuous_rx(struct ipw_priv *priv, > > > > > > /* Zero the flags, we'll add to them as we go */ > > > ipw_rt->rt_flags = 0; > > > - ipw_rt->rt_tsf = (u64)(frame->parent_tsf[3] << 24 | > > > - frame->parent_tsf[2] << 16 | > > > - frame->parent_tsf[1] << 8 | > > > - frame->parent_tsf[0]); > > > + ipw_rt->rt_tsf = (u64)frame->parent_tsf[3] << 24 | > > > + frame->parent_tsf[2] << 16 | > > > + frame->parent_tsf[1] << 8 | > > > + frame->parent_tsf[0]; > > > > > > /* Convert to DBM */ > > > ipw_rt->rt_dbmsignal = signal; > > > > struct ipw_rt_hdr { > > struct ieee80211_radiotap_header rt_hdr; > > u64 rt_tsf; /* TSF */ /* XXX */ > > u8 rt_flags; /* radiotap packet flags * > > u8 rt_rate; /* rate in 500kb/s */ > > __le16 rt_channel; /* channel in mhz */ > > __le16 rt_chbitmask; /* channel bitfield */ > > s8 rt_dbmsignal; /* signal in dbM, kluged to signed */ > > s8 rt_dbmnoise; > > u8 rt_antenna; /* antenna number */ > > u8 payload[0]; /* payload... */ > > } __packed; > > > > Maybe rt_tsf (which is otherwise unused in this code), > > should be __le64 so maybe use (u32) ? > > > > ipw_rt->rt_txf = cpu_to_le64((u32)(frame->parent_tsf[3] << 24 | > > frame->parent_tsf[2] << 16 | > > frame->parent_tsf[1] << 8 | > > frame->parent_tsf[0])); > > > > Hm... It don't think it makes sense to truncate the top bits away by > truncating to u32. You may be right though that there is some larger > bugs here than just the truncation. <shrug> It'd be a tad faster than using multiple 64 bit operations on a 32 bit machine. > Both the "frame" and the "ipw_rt" struct seem to hold little endian > values generally so probably ->rt_txf should be an __le64 like you say. > > Perhaps the maintainers know what should be done? Are there any maintainers left? Likely this was only ever tested/used on x86 hardware. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Gar... I am stupid. The original shift can't wrap. I don't know what I was thinking. Sorry for the noise... Thanks for checking me on this Joe. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Joe, On 24 October 2014 02:43, Joe Perches <joe@perches.com> wrote: > > struct ipw_rt_hdr { > struct ieee80211_radiotap_header rt_hdr; > u64 rt_tsf; /* TSF */ /* XXX */ > u8 rt_flags; /* radiotap packet flags * > u8 rt_rate; /* rate in 500kb/s */ > __le16 rt_channel; /* channel in mhz */ > __le16 rt_chbitmask; /* channel bitfield */ > s8 rt_dbmsignal; /* signal in dbM, kluged to signed */ > s8 rt_dbmnoise; > u8 rt_antenna; /* antenna number */ > u8 payload[0]; /* payload... */ > } __packed; > > Maybe rt_tsf (which is otherwise unused in this code), > should be __le64 so maybe use (u32) ? > Yes, you are right, the field definition should be __le64 as you suggest. All values in radiotap header are specified in little endian byte order according to the documentation at www.radiotap.org. > ipw_rt->rt_txf = cpu_to_le64((u32)(frame->parent_tsf[3] << 24 | > frame->parent_tsf[2] << 16 | > frame->parent_tsf[1] << 8 | > frame->parent_tsf[0])); > That looks fine for me. Will you send a patch? Stanislav. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c index edc3443..7e1f2af 100644 --- a/drivers/net/wireless/ipw2x00/ipw2200.c +++ b/drivers/net/wireless/ipw2x00/ipw2200.c @@ -7819,10 +7819,10 @@ static void ipw_handle_data_packet_monitor(struct ipw_priv *priv, /* Zero the flags, we'll add to them as we go */ ipw_rt->rt_flags = 0; - ipw_rt->rt_tsf = (u64)(frame->parent_tsf[3] << 24 | - frame->parent_tsf[2] << 16 | - frame->parent_tsf[1] << 8 | - frame->parent_tsf[0]); + ipw_rt->rt_tsf = (u64)frame->parent_tsf[3] << 24 | + frame->parent_tsf[2] << 16 | + frame->parent_tsf[1] << 8 | + frame->parent_tsf[0]; /* Convert signal to DBM */ ipw_rt->rt_dbmsignal = antsignal; @@ -8028,10 +8028,10 @@ static void ipw_handle_promiscuous_rx(struct ipw_priv *priv, /* Zero the flags, we'll add to them as we go */ ipw_rt->rt_flags = 0; - ipw_rt->rt_tsf = (u64)(frame->parent_tsf[3] << 24 | - frame->parent_tsf[2] << 16 | - frame->parent_tsf[1] << 8 | - frame->parent_tsf[0]); + ipw_rt->rt_tsf = (u64)frame->parent_tsf[3] << 24 | + frame->parent_tsf[2] << 16 | + frame->parent_tsf[1] << 8 | + frame->parent_tsf[0]; /* Convert to DBM */ ipw_rt->rt_dbmsignal = signal;
The ->parent_tsf[] array holds u8 values. It type promoted to int for the shift operation so the "<< 24" shift operation can wrap. The cast needs to be done before the shift instead of after. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- Static checker work. Untested. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html