[PATCH] fs: fat: Prevent possible buffer overflow

[PATCH] fs: fat: Prevent possible buffer overflow

[Andrew Goodbody]

Instead of strcpy which is unbounded use strlcpy to ensure that the
receiving buffer cannot be overflowed.

This issue found by Smatch.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 fs/fat/fat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 89f2acbba1e..fa34ad7a501 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -1391,7 +1391,7 @@ int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp)
 		return -ENOENT;
 
 	memset(dent, 0, sizeof(*dent));
-	strcpy(dent->name, dir->itr.name);
+	strlcpy(dent->name, dir->itr.name, FS_DIRENT_NAME_LEN);
 	if (CONFIG_IS_ENABLED(EFI_LOADER)) {
 		dent->attr = dir->itr.dent->attr;
 		fat2rtc(le16_to_cpu(dir->itr.dent->cdate),

---
base-commit: 3b4604a40b9fd61b87e9d059fc56f04d36f1a380
change-id: 20250717-fsfat-a77ce8151f74

Best regards,
-- 
Andrew Goodbody <andrew.goodbody@linaro.org>

Re: [PATCH] fs: fat: Prevent possible buffer overflow

[Martin Husemann]

On Thu, Jul 17, 2025 at 03:54:37PM +0100, Andrew Goodbody wrote:
>  
>  	memset(dent, 0, sizeof(*dent));
> -	strcpy(dent->name, dir->itr.name);
> +	strlcpy(dent->name, dir->itr.name, FS_DIRENT_NAME_LEN);

Shouldn't that be strncpy() instead? Using strlcpy() for fixed size
records where strings may not be \0 terminated is quirky.

Martin

Re: [PATCH] fs: fat: Prevent possible buffer overflow

[Andrew Goodbody]

On 17/07/2025 15:58, Martin Husemann wrote:
> On Thu, Jul 17, 2025 at 03:54:37PM +0100, Andrew Goodbody wrote:
>>   
>>   	memset(dent, 0, sizeof(*dent));
>> -	strcpy(dent->name, dir->itr.name);
>> +	strlcpy(dent->name, dir->itr.name, FS_DIRENT_NAME_LEN);
> 
> Shouldn't that be strncpy() instead? Using strlcpy() for fixed size
> records where strings may not be \0 terminated is quirky.
> 
> Martin

Hi Martin,

The original use of strcpy suggests that the string must be \0 
terminated. I will admit that I do not know the code well, is 
dir->itr.name guaranteed to be a known fixed size?

Andrew

Re: [PATCH] fs: fat: Prevent possible buffer overflow

[Martin Husemann]

On Thu, Jul 17, 2025 at 04:38:50PM +0100, Andrew Goodbody wrote:
> The original use of strcpy suggests that the string must be \0 terminated. I
> will admit that I do not know the code well, is dir->itr.name guaranteed to
> be a known fixed size?

You are right, the iterator's name pointer always points to a \0 terminated
string, but also always points to one shorter than FS_DIRENT_NAME_LEN.

The on-disk fields are not \0 terminated, but the code extracting them
deals with it properly. If the length would overflow, a NULL instead
of a dir_entry * is returned, so while it is not obvious, AFAICT the
original code is fine.

Martin

Re: [PATCH] fs: fat: Prevent possible buffer overflow

[Andrew Goodbody]

On 17/07/2025 17:57, Martin Husemann wrote:
> On Thu, Jul 17, 2025 at 04:38:50PM +0100, Andrew Goodbody wrote:
>> The original use of strcpy suggests that the string must be \0 terminated. I
>> will admit that I do not know the code well, is dir->itr.name guaranteed to
>> be a known fixed size?
> 
> You are right, the iterator's name pointer always points to a \0 terminated
> string, but also always points to one shorter than FS_DIRENT_NAME_LEN.
> 
> The on-disk fields are not \0 terminated, but the code extracting them
> deals with it properly. If the length would overflow, a NULL instead
> of a dir_entry * is returned, so while it is not obvious, AFAICT the
> original code is fine.
> 
> Martin

Thanks Martin, we can drop this patch.

Andrew