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 <[email protected]>
---
Static checker work. Untested.
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;
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
Hello, Joe,
On 24 October 2014 02:43, Joe Perches <[email protected]> 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 http://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.
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.
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
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 <[email protected]>
> ---
> 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.