[cifs:for-next 3/8] fs/cifs/connect.c:1303 cifs_ipaddr_cmp() error: memcmp() '&saddr4->sin_addr.s_addr' too small (4 vs 16)
Steve French
smfrench at gmail.com
Thu Mar 23 16:40:26 UTC 2023
I have removed that patch from for-next (so still has the other 8
multichannel fixes and debug improvements). Should be adding an
important deferred close fix later this afternoon and the "umount -f"
fix as well
On Thu, Mar 23, 2023 at 8:07 AM Tom Talpey via samba-technical
<samba-technical at lists.samba.org> wrote:
>
> On 3/23/2023 5:40 AM, Dan Carpenter wrote:
> > tree: git://git.samba.org/sfrench/cifs-2.6.git for-next
> > head: 96114df697dfaef2ce29c14089a83e4a5777e915
> > commit: 010c4e0a894e6a3dee3176aa2f654fce632d0346 [3/8] cifs: fix sockaddr comparison in iface_cmp
> > config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20230323/202303230210.ufS9gVzi-lkp@intel.com/config)
> > compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
> >
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <lkp at intel.com>
> > | Reported-by: Dan Carpenter <error27 at gmail.com>
> > | Link: https://lore.kernel.org/r/202303230210.ufS9gVzi-lkp@intel.com/
> >
> > New smatch warnings:
> > fs/cifs/connect.c:1303 cifs_ipaddr_cmp() error: memcmp() '&saddr4->sin_addr.s_addr' too small (4 vs 16)
> > fs/cifs/connect.c:1318 cifs_ipaddr_cmp() error: memcmp() '&saddr6->sin6_addr' too small (16 vs 28)
> >
> > Old smatch warnings:
> > fs/cifs/connect.c:1303 cifs_ipaddr_cmp() error: memcmp() '&vaddr4->sin_addr.s_addr' too small (4 vs 16)
> > fs/cifs/connect.c:1318 cifs_ipaddr_cmp() error: memcmp() '&vaddr6->sin6_addr' too small (16 vs 28)
> > fs/cifs/connect.c:2937 generic_ip_connect() error: we previously assumed 'socket' could be null (see line 2925)
> >
> > vim +1303 fs/cifs/connect.c
> >
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1279 int
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1280 cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs)
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1281 {
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1282 struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1283 struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1284 struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1285 struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)rhs;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1286
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1287 switch (srcaddr->sa_family) {
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1288 case AF_UNSPEC:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1289 switch (rhs->sa_family) {
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1290 case AF_UNSPEC:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1291 return 0;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1292 case AF_INET:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1293 case AF_INET6:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1294 return 1;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1295 default:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1296 return -1;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1297 }
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1298 case AF_INET: {
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1299 switch (rhs->sa_family) {
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1300 case AF_UNSPEC:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1301 return -1;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1302 case AF_INET:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 @1303 return memcmp(&saddr4->sin_addr.s_addr,
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1304 &vaddr4->sin_addr.s_addr,
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1305 sizeof(struct sockaddr_in));
> >
> > saddr4 and vaddr4 are type sockaddr_in. But sin_addr.s_addr is an
> > offset into the struct. This looks like a read overflow. I would think
> > it should be sizeof(struct in_addr).
>
> Oh, definitely. It's more than a read overflow, it's an incorrect
> comparison which will lead to creating new and unnecessary channels.
> Two bugs here.
>
> Tom.
>
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1306 case AF_INET6:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1307 return 1;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1308 default:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1309 return -1;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1310 }
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1311 }
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1312 case AF_INET6: {
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1313 switch (rhs->sa_family) {
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1314 case AF_UNSPEC:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1315 case AF_INET:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1316 return -1;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1317 case AF_INET6:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 @1318 return memcmp(&saddr6->sin6_addr,
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1319 &vaddr6->sin6_addr,
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1320 sizeof(struct sockaddr_in6));
> >
> > Same.
> >
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1321 default:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1322 return -1;
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1323 }
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1324 }
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1325 default:
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1326 return -1; /* don't expect to be here */
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1327 }
> > 010c4e0a894e6a3 Shyam Prasad N 2022-12-27 1328 }
> >
>
--
Thanks,
Steve
More information about the samba-technical
mailing list