ipw2x00: shift wrap bugs setting ->rt_tsf

PATCH In review Dan Carpenter 3 participants 6 messages Oct 24, 2014 – Oct 28, 2014

Subsystems (2)

Kernel janitors Networking drivers (wireless)

lore

Share & tools

lore

Dan Carpenter

author

2014-10-24 08:15 UTC #0 lore raw

PATCH 1/1 ipw2x00: shift wrap bugs setting ->rt_tsf

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.

drivers/net/wireless/ipw2x00/ipw2200.c +8 -8

index edc3443..7e1f2af 100644
@@ -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;

Joe Perches

2014-10-24 09:43 UTC #1 lore raw

drivers/net/wireless/ipw2x00/ipw2200.c

Quoted 9 lines · Dan Carpenter

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.
drivers/net/wireless/ipw2x00/ipw2200.c +0 -0

[]

Quoted 20 lines

drivers/net/wireless/ipw2x00/ipw2200.c +4 -5

@@ -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 |

drivers/net/wireless/ipw2x00/ipw2200.c static void ipw_handle_promiscuous_rx(struct ipw_priv *priv, · line 8037

Quoted 10 lines

drivers/net/wireless/ipw2x00/ipw2200.c +4 -3

-			       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.

Dan Carpenter

author

2014-10-27 09:52 UTC #2 lore raw

Quoted 30 lines · Joe Perches

Quoted 5 lines · Dan Carpenter

drivers/net/wireless/ipw2x00/ipw2200.c +0 -1

@@ -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 |

drivers/net/wireless/ipw2x00/ipw2200.c static void ipw_handle_promiscuous_rx(struct ipw_priv *priv, · line 8037

Quoted 10 lines

drivers/net/wireless/ipw2x00/ipw2200.c +4 -3

-			       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;

Quoted 8 lines

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

Joe Perches

2014-10-27 15:05 UTC #3 lore raw

Quoted 40 lines · Dan Carpenter

Quoted 30 lines · Joe Perches

Quoted 5 lines · Dan Carpenter

drivers/net/wireless/ipw2x00/ipw2200.c +0 -1

@@ -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 |

drivers/net/wireless/ipw2x00/ipw2200.c static void ipw_handle_promiscuous_rx(struct ipw_priv *priv, · line 8037

Quoted 10 lines

drivers/net/wireless/ipw2x00/ipw2200.c +4 -3

-			       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;

Quoted 8 lines

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]));

Quoted 3 lines

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.

Quoted 4 lines

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.

Dan Carpenter

author

2014-10-27 15:18 UTC #4 lore raw

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

Stanislav Yakovlev

2014-10-28 09:20 UTC #5 lore raw

In reply to Joe Perches

Hello, Joe,

Quoted 14 lines · Joe Perches

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;

Quoted 3 lines

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.

Quoted 5 lines

        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.