* [patch] ipw2x00: shift wrap bugs setting ->rt_tsf
@ 2014-10-24 8:15 Dan Carpenter
2014-10-24 9:43 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2014-10-24 8:15 UTC (permalink / raw)
To: Stanislav Yakovlev; +Cc: John W. Linville, linux-wireless, kernel-janitors
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
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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch] ipw2x00: shift wrap bugs setting ->rt_tsf
2014-10-24 8:15 [patch] ipw2x00: shift wrap bugs setting ->rt_tsf Dan Carpenter
@ 2014-10-24 9:43 ` Joe Perches
2014-10-27 9:52 ` Dan Carpenter
2014-10-28 9:20 ` Stanislav Yakovlev
0 siblings, 2 replies; 6+ messages in thread
From: Joe Perches @ 2014-10-24 9:43 UTC (permalink / raw)
To: Dan Carpenter
Cc: Stanislav Yakovlev, John W. Linville, linux-wireless,
kernel-janitors
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] ipw2x00: shift wrap bugs setting ->rt_tsf
2014-10-24 9:43 ` Joe Perches
@ 2014-10-27 9:52 ` Dan Carpenter
2014-10-27 15:05 ` Joe Perches
2014-10-28 9:20 ` Stanislav Yakovlev
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2014-10-27 9:52 UTC (permalink / raw)
To: Joe Perches
Cc: Stanislav Yakovlev, John W. Linville, linux-wireless,
kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] ipw2x00: shift wrap bugs setting ->rt_tsf
2014-10-27 9:52 ` Dan Carpenter
@ 2014-10-27 15:05 ` Joe Perches
2014-10-27 15:18 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2014-10-27 15:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: Stanislav Yakovlev, John W. Linville, linux-wireless,
kernel-janitors
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] ipw2x00: shift wrap bugs setting ->rt_tsf
2014-10-24 9:43 ` Joe Perches
2014-10-27 9:52 ` Dan Carpenter
@ 2014-10-28 9:20 ` Stanislav Yakovlev
1 sibling, 0 replies; 6+ messages in thread
From: Stanislav Yakovlev @ 2014-10-28 9:20 UTC (permalink / raw)
To: Joe Perches; +Cc: Dan Carpenter, John W. Linville, wireless, kernel-janitors
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-28 9:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-24 8:15 [patch] ipw2x00: shift wrap bugs setting ->rt_tsf Dan Carpenter
2014-10-24 9:43 ` Joe Perches
2014-10-27 9:52 ` Dan Carpenter
2014-10-27 15:05 ` Joe Perches
2014-10-27 15:18 ` Dan Carpenter
2014-10-28 9:20 ` Stanislav Yakovlev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).