From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Hershberger Date: Tue, 31 Mar 2015 12:48:57 -0500 Subject: [U-Boot] [PATCH] net: Fix incorrect DHCP/BOOTP packet layout on 64-bit systems In-Reply-To: <1427822251-32521-1-git-send-email-s.temerkhanov@gmail.com> References: <1427822251-32521-1-git-send-email-s.temerkhanov@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Sergey, On Tue, Mar 31, 2015 at 12:17 PM, Sergey Temerkhanov < s.temerkhanov@gmail.com> wrote: > > > This commit fixes incorrect DHCP/BOOTP packet layout caused by > 'ulong' type size difference on 64 and 32-bit architectures. > It also converts protocol header structures to use explicitly > sized fields and renames NetReadLong()/NetCopyLong() to > NetReadU32/NetCopyU32() accordingly > > Signed-off-by: Radha Mohan Chintakuntla > Signed-off-by: Sergey Temerkhanov > > Cc: joe.hershberger at gmail.com > > > --- > include/net.h | 122 +++++++++++++++++++++++++++++----------------------------- > net/bootp.c | 24 ++++++------ > net/bootp.h | 24 ++++++------ > 3 files changed, 85 insertions(+), 85 deletions(-) > > diff --git a/include/net.h b/include/net.h > index 237c932..588ab7d 100644 > --- a/include/net.h > +++ b/include/net.h > @@ -183,9 +183,9 @@ u32 ether_crc(size_t len, unsigned char const *p); > */ > > struct ethernet_hdr { > - uchar et_dest[6]; /* Destination node */ > - uchar et_src[6]; /* Source node */ > - ushort et_protlen; /* Protocol or length */ > + u8 et_dest[6]; /* Destination node */ > + u8 et_src[6]; /* Source node */ > + u16 et_protlen; /* Protocol or length */ > }; > > /* Ethernet header size */ > @@ -194,16 +194,16 @@ struct ethernet_hdr { > #define ETH_FCS_LEN 4 /* Octets in the FCS */ > > struct e802_hdr { > - uchar et_dest[6]; /* Destination node */ > - uchar et_src[6]; /* Source node */ > - ushort et_protlen; /* Protocol or length */ > - uchar et_dsap; /* 802 DSAP */ > - uchar et_ssap; /* 802 SSAP */ > - uchar et_ctl; /* 802 control */ > - uchar et_snap1; /* SNAP */ > - uchar et_snap2; > - uchar et_snap3; > - ushort et_prot; /* 802 protocol */ > + u8 et_dest[6]; /* Destination node */ > + u8 et_src[6]; /* Source node */ > + u16 et_protlen; /* Protocol or length */ > + u8 et_dsap; /* 802 DSAP */ > + u8 et_ssap; /* 802 SSAP */ > + u8 et_ctl; /* 802 control */ > + u8 et_snap1; /* SNAP */ > + u8 et_snap2; > + u8 et_snap3; > + u16 et_prot; /* 802 protocol */ > }; > > /* 802 + SNAP + ethernet header size */ > @@ -213,11 +213,11 @@ struct e802_hdr { > * Virtual LAN Ethernet header > */ > struct vlan_ethernet_hdr { > - uchar vet_dest[6]; /* Destination node */ > - uchar vet_src[6]; /* Source node */ > - ushort vet_vlan_type; /* PROT_VLAN */ > - ushort vet_tag; /* TAG of VLAN */ > - ushort vet_type; /* protocol type */ > + u8 vet_dest[6]; /* Destination node */ > + u8 vet_src[6]; /* Source node */ > + u16 vet_vlan_type; /* PROT_VLAN */ > + u16 vet_tag; /* TAG of VLAN */ > + u16 vet_type; /* protocol type */ > }; > > /* VLAN Ethernet header size */ > @@ -235,14 +235,14 @@ struct vlan_ethernet_hdr { > * Internet Protocol (IP) header. > */ > struct ip_hdr { > - uchar ip_hl_v; /* header length and version */ > - uchar ip_tos; /* type of service */ > - ushort ip_len; /* total length */ > - ushort ip_id; /* identification */ > - ushort ip_off; /* fragment offset field */ > - uchar ip_ttl; /* time to live */ > - uchar ip_p; /* protocol */ > - ushort ip_sum; /* checksum */ > + u8 ip_hl_v; /* header length and version */ > + u8 ip_tos; /* type of service */ > + u16 ip_len; /* total length */ > + u16 ip_id; /* identification */ > + u16 ip_off; /* fragment offset field */ > + u8 ip_ttl; /* time to live */ > + u8 ip_p; /* protocol */ > + u16 ip_sum; /* checksum */ > IPaddr_t ip_src; /* Source IP address */ > IPaddr_t ip_dst; /* Destination IP address */ > }; > @@ -259,20 +259,20 @@ struct ip_hdr { > * Internet Protocol (IP) + UDP header. > */ > struct ip_udp_hdr { > - uchar ip_hl_v; /* header length and version */ > - uchar ip_tos; /* type of service */ > - ushort ip_len; /* total length */ > - ushort ip_id; /* identification */ > - ushort ip_off; /* fragment offset field */ > - uchar ip_ttl; /* time to live */ > - uchar ip_p; /* protocol */ > - ushort ip_sum; /* checksum */ > + u8 ip_hl_v; /* header length and version */ > + u8 ip_tos; /* type of service */ > + u16 ip_len; /* total length */ > + u16 ip_id; /* identification */ > + u16 ip_off; /* fragment offset field */ > + u8 ip_ttl; /* time to live */ > + u8 ip_p; /* protocol */ > + u16 ip_sum; /* checksum */ > IPaddr_t ip_src; /* Source IP address */ > IPaddr_t ip_dst; /* Destination IP address */ > - ushort udp_src; /* UDP source port */ > - ushort udp_dst; /* UDP destination port */ > - ushort udp_len; /* Length of UDP packet */ > - ushort udp_xsum; /* Checksum */ > + u16 udp_src; /* UDP source port */ > + u16 udp_dst; /* UDP destination port */ > + u16 udp_len; /* Length of UDP packet */ > + u16 udp_xsum; /* Checksum */ > }; > > #define IP_UDP_HDR_SIZE (sizeof(struct ip_udp_hdr)) > @@ -282,14 +282,14 @@ struct ip_udp_hdr { > * Address Resolution Protocol (ARP) header. > */ > struct arp_hdr { > - ushort ar_hrd; /* Format of hardware address */ > + u16 ar_hrd; /* Format of hardware address */ > # define ARP_ETHER 1 /* Ethernet hardware address */ > - ushort ar_pro; /* Format of protocol address */ > - uchar ar_hln; /* Length of hardware address */ > + u16 ar_pro; /* Format of protocol address */ > + u8 ar_hln; /* Length of hardware address */ > # define ARP_HLEN 6 > - uchar ar_pln; /* Length of protocol address */ > + u8 ar_pln; /* Length of protocol address */ > # define ARP_PLEN 4 > - ushort ar_op; /* Operation */ > + u16 ar_op; /* Operation */ > # define ARPOP_REQUEST 1 /* Request to resolve address */ > # define ARPOP_REPLY 2 /* Response to previous request */ > > @@ -301,16 +301,16 @@ struct arp_hdr { > * the sizes above, and are defined as appropriate for > * specific hardware/protocol combinations. > */ > - uchar ar_data[0]; > + u8 ar_data[0]; > #define ar_sha ar_data[0] > #define ar_spa ar_data[ARP_HLEN] > #define ar_tha ar_data[ARP_HLEN + ARP_PLEN] > #define ar_tpa ar_data[ARP_HLEN + ARP_PLEN + ARP_HLEN] > #if 0 > - uchar ar_sha[]; /* Sender hardware address */ > - uchar ar_spa[]; /* Sender protocol address */ > - uchar ar_tha[]; /* Target hardware address */ > - uchar ar_tpa[]; /* Target protocol address */ > + u8 ar_sha[]; /* Sender hardware address */ > + u8 ar_spa[]; /* Sender protocol address */ > + u8 ar_tha[]; /* Target hardware address */ > + u8 ar_tpa[]; /* Target protocol address */ > #endif /* 0 */ > }; > > @@ -332,20 +332,20 @@ struct arp_hdr { > #define ICMP_NOT_REACH_PORT 3 /* Port unreachable */ > > struct icmp_hdr { > - uchar type; > - uchar code; > - ushort checksum; > + u8 type; > + u8 code; > + u16 checksum; > union { > struct { > - ushort id; > - ushort sequence; > + u16 id; > + u16 sequence; > } echo; > - ulong gateway; > + u32 gateway; > struct { > - ushort unused; > - ushort mtu; > + u16 unused; > + u16 mtu; > } frag; > - uchar data[0]; > + u8 data[0]; > } un; > }; > > @@ -608,9 +608,9 @@ static inline IPaddr_t NetReadIP(void *from) > } > > /* return ulong *in network byteorder* */ > -static inline ulong NetReadLong(ulong *from) > +static inline u32 NetReadU32(u32 *from) > { > - ulong l; > + u32 l; > > memcpy((void *)&l, (void *)from, sizeof(l)); > return l; > @@ -629,9 +629,9 @@ static inline void NetCopyIP(void *to, void *from) > } > > /* copy ulong */ > -static inline void NetCopyLong(ulong *to, ulong *from) > +static inline void NetCopyU32(u32 *to, u32 *from) This surely causes a checkpatch.pl warning. While you are touching this, you should name it "net_copy_u32()". > { > - memcpy((void *)to, (void *)from, sizeof(ulong)); > + memcpy((void *)to, (void *)from, sizeof(u32)); > } > > /** > diff --git a/net/bootp.c b/net/bootp.c > index 8106601..8ce0bdd 100644 > --- a/net/bootp.c > +++ b/net/bootp.c > @@ -51,7 +51,7 @@ > #define CONFIG_BOOTP_ID_CACHE_SIZE 4 > #endif > > -ulong bootp_ids[CONFIG_BOOTP_ID_CACHE_SIZE]; > +u32 bootp_ids[CONFIG_BOOTP_ID_CACHE_SIZE]; > unsigned int bootp_num_ids; > int BootpTry; > ulong bootp_start; > @@ -59,7 +59,7 @@ ulong bootp_timeout; > > #if defined(CONFIG_CMD_DHCP) > static dhcp_state_t dhcp_state = INIT; > -static unsigned long dhcp_leasetime; > +static u32 dhcp_leasetime; > static IPaddr_t NetDHCPServerIP; > static void DhcpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, > unsigned len); > @@ -125,7 +125,7 @@ static int BootpCheckPkt(uchar *pkt, unsigned dest, unsigned src, unsigned len) > retval = -4; > else if (bp->bp_hlen != HWL_ETHER) > retval = -5; > - else if (!bootp_match_id(NetReadLong((ulong *)&bp->bp_id))) > + else if (!bootp_match_id(NetReadU32(&bp->bp_id))) > retval = -6; > > debug("Filtering pkt = %d\n", retval); > @@ -350,7 +350,7 @@ BootpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, > BootpCopyNetParams(bp); /* Store net parameters from reply */ > > /* Retrieve extended information (we must parse the vendor area) */ > - if (NetReadLong((ulong *)&bp->bp_vend[0]) == htonl(BOOTP_VENDOR_MAGIC)) > + if (NetReadU32((u32 *)&bp->bp_vend[0]) == htonl(BOOTP_VENDOR_MAGIC)) > BootpVendorProcess((uchar *)&bp->bp_vend[4], len); > > NetSetTimeout(0, (thand_f *)0); > @@ -661,7 +661,7 @@ BootpRequest(void) > #ifdef CONFIG_BOOTP_RANDOM_DELAY > ulong rand_ms; > #endif > - ulong BootpID; > + u32 BootpID; Please rename this to "bootp_id". > > bootstage_mark_name(BOOTSTAGE_ID_BOOTP_START, "bootp_start"); > #if defined(CONFIG_CMD_DHCP) > @@ -732,7 +732,7 @@ BootpRequest(void) > BootpID += get_timer(0); > BootpID = htonl(BootpID); > bootp_add_id(BootpID); > - NetCopyLong(&bp->bp_id, &BootpID); > + NetCopyU32(&bp->bp_id, &BootpID); > > /* > * Calculate proper packet lengths taking into account the > @@ -770,7 +770,7 @@ static void DhcpOptionsProcess(uchar *popt, struct Bootp_t *bp) > #if defined(CONFIG_CMD_SNTP) && defined(CONFIG_BOOTP_TIMEOFFSET) > case 2: /* Time offset */ > to_ptr = &NetTimeOffset; > - NetCopyLong((ulong *)to_ptr, (ulong *)(popt + 2)); > + NetCopyU32((u32 *)to_ptr, (u32 *)(popt + 2)); > NetTimeOffset = ntohl(NetTimeOffset); > break; > #endif > @@ -806,7 +806,7 @@ static void DhcpOptionsProcess(uchar *popt, struct Bootp_t *bp) > break; > #endif > case 51: > - NetCopyLong(&dhcp_leasetime, (ulong *) (popt + 2)); > + NetCopyU32(&dhcp_leasetime, (u32 *) (popt + 2)); > break; > case 53: /* Ignore Message Type Option */ > break; > @@ -860,7 +860,7 @@ static void DhcpOptionsProcess(uchar *popt, struct Bootp_t *bp) > > static int DhcpMessageType(unsigned char *popt) > { > - if (NetReadLong((ulong *)popt) != htonl(BOOTP_VENDOR_MAGIC)) > + if (NetReadU32((u32 *)popt) != htonl(BOOTP_VENDOR_MAGIC)) > return -1; > > popt += 4; > @@ -911,7 +911,7 @@ static void DhcpSendRequestPkt(struct Bootp_t *bp_offer) > * ID is the id of the OFFER packet > */ > > - NetCopyLong(&bp->bp_id, &bp_offer->bp_id); > + NetCopyU32(&bp->bp_id, &bp_offer->bp_id); > > /* > * Copy options from OFFER packet if present > @@ -970,7 +970,7 @@ DhcpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, > debug("TRANSITIONING TO REQUESTING STATE\n"); > dhcp_state = REQUESTING; > > - if (NetReadLong((ulong *)&bp->bp_vend[0]) == > + if (NetReadU32((u32 *)&bp->bp_vend[0]) == > htonl(BOOTP_VENDOR_MAGIC)) > DhcpOptionsProcess((u8 *)&bp->bp_vend[4], bp); > > @@ -986,7 +986,7 @@ DhcpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, > debug("DHCP State: REQUESTING\n"); > > if (DhcpMessageType((u8 *)bp->bp_vend) == DHCP_ACK) { > - if (NetReadLong((ulong *)&bp->bp_vend[0]) == > + if (NetReadU32((u32 *)&bp->bp_vend[0]) == > htonl(BOOTP_VENDOR_MAGIC)) > DhcpOptionsProcess((u8 *)&bp->bp_vend[4], bp); > /* Store net params from reply */ > diff --git a/net/bootp.h b/net/bootp.h > index 3b95a0a..a771655 100644 > --- a/net/bootp.h > +++ b/net/bootp.h > @@ -30,25 +30,25 @@ extern u8 *dhcp_vendorex_proc(u8 *e); /*rtn next e if mine,else NULL */ > #endif > > struct Bootp_t { > - uchar bp_op; /* Operation */ > + u8 bp_op; /* Operation */ > # define OP_BOOTREQUEST 1 > # define OP_BOOTREPLY 2 > - uchar bp_htype; /* Hardware type */ > + u8 bp_htype; /* Hardware type */ > # define HWT_ETHER 1 > - uchar bp_hlen; /* Hardware address length */ > + u8 bp_hlen; /* Hardware address length */ > # define HWL_ETHER 6 > - uchar bp_hops; /* Hop count (gateway thing) */ > - ulong bp_id; /* Transaction ID */ > - ushort bp_secs; /* Seconds since boot */ > - ushort bp_spare1; /* Alignment */ > + u8 bp_hops; /* Hop count (gateway thing) */ > + u32 bp_id; /* Transaction ID */ > + u16 bp_secs; /* Seconds since boot */ > + u16 bp_spare1; /* Alignment */ > IPaddr_t bp_ciaddr; /* Client IP address */ > IPaddr_t bp_yiaddr; /* Your (client) IP address */ > IPaddr_t bp_siaddr; /* Server IP address */ > IPaddr_t bp_giaddr; /* Gateway IP address */ > - uchar bp_chaddr[16]; /* Client hardware address */ > - char bp_sname[64]; /* Server host name */ > - char bp_file[128]; /* Boot file name */ > - char bp_vend[OPT_FIELD_SIZE]; /* Vendor information */ > + char bp_chaddr[16]; /* Client hardware address */ > + char bp_sname[64]; /* Server host name */ > + char bp_file[128]; /* Boot file name */ > + char bp_vend[OPT_FIELD_SIZE]; /* Vendor information */ > }; > > #define BOOTP_HDR_SIZE sizeof(struct Bootp_t) > @@ -59,7 +59,7 @@ struct Bootp_t { > */ > > /* bootp.c */ > -extern ulong BootpID; /* ID of cur BOOTP request */ > +extern u32 BootpID; /* ID of cur BOOTP request */ > extern char BootFile[128]; /* Boot file name */ > extern int BootpTry; I like what you're doing here, but I'm doing a lot of refactoring in this area (including most of this). I'm just going to pull this patch into the series I'm working on right now and make the changes I suggested. I'll be sending a refactoring series out in the next few weeks that will include this. Thanks, -Joe