"CIFS: VFS: directory entry name would overflow frame end of buf" and invisible files under certain conditions and at least with noserverino mount option

Hi Paulo, hi all

In Debian we got two reports of cifs mounts not functioning, hiding certain files. The two reports are:

https://bugs.debian.org/1069102
https://bugs.debian.org/1069092

On those cases kernel logs error

[   23.225952] CIFS: VFS: directory entry name would overflow frame end of buf 00000000a44b272c

I do not have yet a minimal reproducing setup, but I was able to reproduce the the issue cerating a simple share (done for simplicity with ksmbd):

[global]

	...

[poc]
path = /srv/data
valid users = root
read only = no

Within /srv/data create an empty file libfoo:

# touch /srv/data/libfoo

The share is mounted with noserverino (the issue is not reproducible without at least in my case):

mount -t cifs -o noserverino //server/poc /mnt

On each access of /mnt a new error is logged, while not showing the libfoo file:

[   23.225952] CIFS: VFS: directory entry name would overflow frame end of buf 00000000a44b272c
[  603.494356] CIFS: VFS: directory entry name would overflow frame end of buf 000000001dbf54e1
[  633.217689] CIFS: VFS: directory entry name would overflow frame end of buf 00000000fb4597c4
[  642.791862] CIFS: VFS: directory entry name would overflow frame end of buf 0000000023b48528

I have verified that reverting in 6.1.y the commit 0947d0d463d4 ("smb:
client: set correct d_type for reparse points under DFS mounts") on top of 6.1.87 fixes the issue.

#regzbot introduced: 0947d0d463d4

I can try to make a clean environment to reproeduce the issue, but I'm not yet there. But the regression is related to 0947d0d463d4 ("smb:
client: set correct d_type for reparse points under DFS mounts"). The mentioned commit was as well part of 6.7.7 at least, but I'm not able to reproduce the issue from another client running 6.7,9.

Does that ring some bell?

Regards,
Salvatore

Hi Salvatore,

I couldn't reproduce it. Does the following fix your issue:

index 4c1231496a72..3ee35430595e 100644
@@ -5083,7 +5083,7 @@ smb2_parse_query_directory(struct cifs_tcon *tcon,
 		info_buf_size = sizeof(struct smb2_posix_info);
 		break;
 	case SMB_FIND_FILE_FULL_DIRECTORY_INFO:
-		info_buf_size = sizeof(FILE_FULL_DIRECTORY_INFO);
+		info_buf_size = sizeof(FILE_FULL_DIRECTORY_INFO) - 1;
 		break;
 	default:
 		cifs_tcon_dbg(VFS, "info level %u isn't supported\n",

If not, please provide network trace and verbose logs.

Thanks.

Hi Paulo,

Thanks a lot for your time on looking into it.

Yes that appears to fix the issue.

But as you say you are not able to reproduce the issue, I guess we need to try to get it clearly reproducible first to see we face no other fallouts?

Regards,
Salvatore

Thanks for quickly testing it. So the above change indicates that we're missing 35235e19b393 ("cifs: Replace remaining 1-element arrays") in v6.1.y.

Can you test it now with 35235e19b393 backported without the above change?

I couldn't reproduce it in v6.9-rc4. Forgot to mention it, sorry.

Yes, further testing would be great to make sure we're not missing anything else.

Hi Paulo,

Done. From the experiment in the avialable setup this seems to indeed fix the issue. The commit can mostly be cherry-picked with one manual whitespace caused fixup.

Ack understand.

I'm still failing to provide you a recipe with a minimal as possible setup, but with the instance I was able to reproduce the issue the regression seems gone with cherry-picking 35235e19b393 ("cifs: Replace remaining 1-element arrays") .

Regards,
Salvatore

It's OK, no problem. Could you please provide the backport to stable team?

Hi Paulo,

Sure, here it is. Greg or Sasha is it ok to pick that up for the 6.1.y queues?

Regards,
Salvatore

From 515673c8dfed6cdc2ae1d7831cf884b578228d22 Mon Sep 17 00:00:00 2001
From: Kees Cook <[email protected]>
Date: Tue, 14 Feb 2023 16:09:45 -0800
Subject: [PATCH] cifs: Replace remaining 1-element arrays

commit 35235e19b393b54db0e0d7c424d658ba45f20468 upstream.

The kernel is globally removing the ambiguous 0-length and 1-element arrays in favor of flexible arrays, so that we can gain both compile-time and run-time array bounds checking[1].

Replace the trailing 1-element array with a flexible array in the following structures:

struct cifs_spnego_msg
struct cifs_quota_data
struct get_dfs_referral_rsp
struct file_alt_name_info
NEGOTIATE_RSP
SESSION_SETUP_ANDX
TCONX_REQ
TCONX_RSP
TCONX_RSP_EXT
ECHO_REQ
ECHO_RSP
OPEN_REQ
OPENX_REQ
LOCK_REQ
RENAME_REQ
COPY_REQ
COPY_RSP
NT_RENAME_REQ
DELETE_FILE_REQ
DELETE_DIRECTORY_REQ
CREATE_DIRECTORY_REQ
QUERY_INFORMATION_REQ
SETATTR_REQ
TRANSACT_IOCTL_REQ
TRANSACT_CHANGE_NOTIFY_REQ
TRANSACTION2_QPI_REQ
TRANSACTION2_SPI_REQ
TRANSACTION2_FFIRST_REQ
TRANSACTION2_GET_DFS_REFER_REQ
FILE_UNIX_LINK_INFO
FILE_DIRECTORY_INFO
FILE_FULL_DIRECTORY_INFO
SEARCH_ID_FULL_DIR_INFO
FILE_BOTH_DIRECTORY_INFO
FIND_FILE_STANDARD_INFO

Replace the trailing 1-element array with a flexible array, but leave the existing structure padding:

FILE_ALL_INFO
FILE_UNIX_INFO

Remove unused structures:

struct gea
struct gealist

Adjust all related size calculations to match the changes to sizeof().

No machine code output differences are produced after these changes.

[1] For lots of details, see both:
https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
https://people.kernel.org/kees/bounded-flexible-arrays-in-c

[ Salvatore Bonaccorso: Patch does not apply cleanly only due to a
whitespace difference in fs/smb/client/cifspdu.h . Fixed up manually. ]

---
 fs/smb/client/cifs_spnego.h |  2 +-
 fs/smb/client/cifspdu.h     | 96 ++++++++++++++++++-------------------
 fs/smb/client/readdir.c     |  6 +--
 fs/smb/client/smb2pdu.c     |  4 +-
 fs/smb/client/smb2pdu.h     |  2 +-
 5 files changed, 53 insertions(+), 57 deletions(-)

index 7f102ffeb675..e4d751b0c812 100644
@@ -24,7 +24,7 @@ struct cifs_spnego_msg {
 	uint32_t	flags;
 	uint32_t	sesskey_len;
 	uint32_t	secblob_len;
-	uint8_t		data[1];
+	uint8_t		data[];
 };
 
 #ifdef __KERNEL__

index 5990bdbae598..9a1f1913fb59 100644
@@ -497,7 +497,7 @@ static char *nxt_dir_entry(char *old_entry, char *end_of_smb, int level)
 		FIND_FILE_STANDARD_INFO *pfData;
 		pfData = (FIND_FILE_STANDARD_INFO *)pDirInfo;
 
-		new_entry = old_entry + sizeof(FIND_FILE_STANDARD_INFO) +
+		new_entry = old_entry + sizeof(FIND_FILE_STANDARD_INFO) + 1 +
 				pfData->FileNameLength;
 	} else {
 		u32 next_offset = le32_to_cpu(pDirInfo->NextEntryOffset);

@@ -515,9 +515,9 @@ static char *nxt_dir_entry(char *old_entry, char *end_of_smb, int level)
 			 new_entry, end_of_smb, old_entry);
 		return NULL;
 	} else if (((level == SMB_FIND_FILE_INFO_STANDARD) &&
-		    (new_entry + sizeof(FIND_FILE_STANDARD_INFO) > end_of_smb))
+		    (new_entry + sizeof(FIND_FILE_STANDARD_INFO) + 1 > end_of_smb))
 		  || ((level != SMB_FIND_FILE_INFO_STANDARD) &&
-		   (new_entry + sizeof(FILE_DIRECTORY_INFO) > end_of_smb)))  {
+		   (new_entry + sizeof(FILE_DIRECTORY_INFO) + 1 > end_of_smb)))  {
 		cifs_dbg(VFS, "search entry %p extends after end of SMB %p\n",
 			 new_entry, end_of_smb);
 		return NULL;

index cc425a616899..e15bf116c755 100644
@@ -5073,10 +5073,10 @@ smb2_parse_query_directory(struct cifs_tcon *tcon,
 
 	switch (srch_inf->info_level) {
 	case SMB_FIND_FILE_DIRECTORY_INFO:
-		info_buf_size = sizeof(FILE_DIRECTORY_INFO) - 1;
+		info_buf_size = sizeof(FILE_DIRECTORY_INFO);
 		break;
 	case SMB_FIND_FILE_ID_FULL_DIR_INFO:
-		info_buf_size = sizeof(SEARCH_ID_FULL_DIR_INFO) - 1;
+		info_buf_size = sizeof(SEARCH_ID_FULL_DIR_INFO);
 		break;
 	case SMB_FIND_FILE_POSIX_INFO:
 		/* note that posix payload are variable size */

index 8d011fedecd0..3a13b9b56452 100644
@@ -373,7 +373,7 @@ struct smb2_file_id_extd_directory_info {
 	__le32 EaSize; /* EA size */
 	__le32 ReparsePointTag; /* valid if FILE_ATTR_REPARSE_POINT set in FileAttributes */
 	__le64 UniqueId; /* inode num - le since Samba puts ino in low 32 bit */
-	char FileName[1];
+	char FileName[];
 } __packed; /* level 60 */
 
 extern char smb2_padding[7];

2.43.0

Glad to, for some reason I thought this caused problems, but if it passes your testing, great! I'll go queue it up now, thanks.

greg k-h

Hi,

Thanks! Unfortunately I'm not having a good test(case/suite) for this myself for such cases. All issues recently forwarded as regressions in the 6.1.y series were unfortunately only uncovered by Debian users once we did release the version :(. Hopefully this will "calm down" now.

Thanks to all of you, for your work!

Regards,
Salvatore