Differential D8445
Allocate a struct ifreq rather than using a computed size for the BIOCSETIF ioctl. Authored by brooks on Nov 4 2016, 10:49 PM. Tags None Referenced Files
Subscribers None
Details The kernel always copies an entire struct ifreq and IPv4 addresses will On systems with pointers larger than 64-bits, the computed size will be Obtained from: CheriBSD
Diff Detail
Event TimelineComment Actions Mmm, so the issue is that ifru_data can be too large? Note that we have a _SIZEOF_ADDR_IFREQ() helper macro that would seem to need fixing as it assumes that sizeof(sockaddr) > sizeof(caddr_t). I think I'd rather fix this by doing something like: int len = MAX(IFNAMSIZ + ifa->ifa_addr->sa_len, sizeof(struct ifreq)); Alternatively, a variant of _SIZEOF_ADDR_IFREQ() that accepts the 'sa_len' instead of the 'ifr' could be added to <net/if.h> and then used here. Comment Actions The kernel always copies in sizeof(struct ifreq) because that's encoded in the ioctl macro. It would be OK to allocate more, but wouldn't do anything useful without code I don't think exists, particularly since ifa->ifa_addr is an IPv4 address. We could choose to retain a guard against ifa->ifa_addr->sa_len becoming too long due to refactoring, but I think it should be something like this: assert(offsetof(struct ifreq, ifr_ifru) + ifa->ifa_addr->sa_len <= sizeof(struct ifreq)) The previous calculation is wrong because it assumes ifr_ifru has alignment <= IFNAMSIZ which isn't necessarily true. The somewhat diverged code in OpenBSD just allocates a struct ifreq. Note: I found the bug because a CHERI kernel noticed that the allocation was too small and thus rejected the ioctl() call. Comment Actions Ugh, this is such a fail (the code in general). So we have 'struct ifreq', but it turns out that is IPv4-only. IPv6 sockets use a 'struct in6_ifreq' and use SIO<blahblah>_IN6 ioctls instead. The "correct" interface would seem to be that the kernel should copyin the ifreq and if it is a request that uses ifr_addr or one of the other fields, check sa_len and copy in the full length (or ifreq should use 'sockaddr_storage'). However, we don't do either of those. The _SIZEOF_ADDR_IFREQ() macro is actually smarter about alignment, etc. as it just adds the extra bytes in the sockaddr to the size of 'struct ifreq' itself if needed. However, given that we don't actually handle sa_len for real it seems like we should remove that macro instead. |