Fix attached. Harmless in some sense because lengths are the same, but is easier to read/understand with the change you suggest. On Fri, Sep 17, 2021 at 9:19 AM Dan Carpenter wrote: > > Hello Ronnie Sahlberg, > > The patch ea64370bcae1: "cifs: refactor create_sd_buf() and and avoid > corrupting the buffer" from Nov 30, 2020, leads to the following > Smatch static checker warning: > > fs/smbfs_client/smb2pdu.c:2425 create_sd_buf() > warn: struct type mismatch 'smb3_acl vs cifs_acl' > > fs/smbfs_client/smb2pdu.c > 2344 static struct crt_sd_ctxt * > 2345 create_sd_buf(umode_t mode, bool set_owner, unsigned int *len) > 2346 { > 2347 struct crt_sd_ctxt *buf; > 2348 __u8 *ptr, *aclptr; > 2349 unsigned int acelen, acl_size, ace_count; > 2350 unsigned int owner_offset = 0; > 2351 unsigned int group_offset = 0; > 2352 struct smb3_acl acl; > ^^^^^^^^^^^^^^^^^^^ > > 2353 > 2354 *len = roundup(sizeof(struct crt_sd_ctxt) + (sizeof(struct cifs_ace) * 4), 8); > 2355 > 2356 if (set_owner) { > 2357 /* sizeof(struct owner_group_sids) is already multiple of 8 so no need to round */ > 2358 *len += sizeof(struct owner_group_sids); > 2359 } > 2360 > 2361 buf = kzalloc(*len, GFP_KERNEL); > 2362 if (buf == NULL) > 2363 return buf; > 2364 > 2365 ptr = (__u8 *)&buf[1]; > 2366 if (set_owner) { > 2367 /* offset fields are from beginning of security descriptor not of create context */ > 2368 owner_offset = ptr - (__u8 *)&buf->sd; > 2369 buf->sd.OffsetOwner = cpu_to_le32(owner_offset); > 2370 group_offset = owner_offset + offsetof(struct owner_group_sids, group); > 2371 buf->sd.OffsetGroup = cpu_to_le32(group_offset); > 2372 > 2373 setup_owner_group_sids(ptr); > 2374 ptr += sizeof(struct owner_group_sids); > 2375 } else { > 2376 buf->sd.OffsetOwner = 0; > 2377 buf->sd.OffsetGroup = 0; > 2378 } > 2379 > 2380 buf->ccontext.DataOffset = cpu_to_le16(offsetof(struct crt_sd_ctxt, sd)); > 2381 buf->ccontext.NameOffset = cpu_to_le16(offsetof(struct crt_sd_ctxt, Name)); > 2382 buf->ccontext.NameLength = cpu_to_le16(4); > 2383 /* SMB2_CREATE_SD_BUFFER_TOKEN is "SecD" */ > 2384 buf->Name[0] = 'S'; > 2385 buf->Name[1] = 'e'; > 2386 buf->Name[2] = 'c'; > 2387 buf->Name[3] = 'D'; > 2388 buf->sd.Revision = 1; /* Must be one see MS-DTYP 2.4.6 */ > 2389 > 2390 /* > 2391 * ACL is "self relative" ie ACL is stored in contiguous block of memory > 2392 * and "DP" ie the DACL is present > 2393 */ > 2394 buf->sd.Control = cpu_to_le16(ACL_CONTROL_SR | ACL_CONTROL_DP); > 2395 > 2396 /* offset owner, group and Sbz1 and SACL are all zero */ > 2397 buf->sd.OffsetDacl = cpu_to_le32(ptr - (__u8 *)&buf->sd); > 2398 /* Ship the ACL for now. we will copy it into buf later. */ > 2399 aclptr = ptr; > 2400 ptr += sizeof(struct cifs_acl); > 2401 > 2402 /* create one ACE to hold the mode embedded in reserved special SID */ > 2403 acelen = setup_special_mode_ACE((struct cifs_ace *)ptr, (__u64)mode); > 2404 ptr += acelen; > 2405 acl_size = acelen + sizeof(struct smb3_acl); > ^^^^^^^^^^^^^^^ > 2406 ace_count = 1; > 2407 > 2408 if (set_owner) { > 2409 /* we do not need to reallocate buffer to add the two more ACEs. plenty of space */ > 2410 acelen = setup_special_user_owner_ACE((struct cifs_ace *)ptr); > 2411 ptr += acelen; > 2412 acl_size += acelen; > 2413 ace_count += 1; > 2414 } > 2415 > 2416 /* and one more ACE to allow access for authenticated users */ > 2417 acelen = setup_authusers_ACE((struct cifs_ace *)ptr); > 2418 ptr += acelen; > 2419 acl_size += acelen; > 2420 ace_count += 1; > 2421 > 2422 acl.AclRevision = ACL_REVISION; /* See 2.4.4.1 of MS-DTYP */ > 2423 acl.AclSize = cpu_to_le16(acl_size); > 2424 acl.AceCount = cpu_to_le16(ace_count); > --> 2425 memcpy(aclptr, &acl, sizeof(struct cifs_acl)); > ^^^^^^^^^^^^^^^^^^^^^^^ > smb3_acl and cifs_acl structs are both 8 bytes, but their data is quite > different. > > (This old warnings are all showing up as new warnings because the file > was moved). > > 2426 > 2427 buf->ccontext.DataLength = cpu_to_le32(ptr - (__u8 *)&buf->sd); > 2428 *len = roundup(ptr - (__u8 *)buf, 8); > 2429 > 2430 return buf; > 2431 } > > regards, > dan carpenter -- Thanks, Steve