Skip to content

Commit

Permalink
patches: Add workaround for fortify warning in fs/smb/client/cifsencr…
Browse files Browse the repository at this point in the history
…ypt.c

Link: ClangBuiltLinux/linux#1966
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
  • Loading branch information
nathanchance committed Jan 25, 2024
1 parent 8adc52b commit dcd26b9
Show file tree
Hide file tree
Showing 12 changed files with 414 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
From git@z Thu Jan 1 00:00:00 1970
Subject: [PATCH] smb: Work around Clang __bdos() type confusion
From: Kees Cook <keescook@chromium.org>
Date: Tue, 23 Jan 2024 15:47:34 -0800
Message-Id: <20240123234731.work.358-kees@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit

Recent versions of Clang gets confused about the possible size of the
"user" allocation, and CONFIG_FORTIFY_SOURCE ends up emitting a
warning[1]:

repro.c:126:4: warning: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
126 | __write_overflow_field(p_size_field, size);
| ^

for this memset():

int len;
__le16 *user;
...
len = ses->user_name ? strlen(ses->user_name) : 0;
user = kmalloc(2 + (len * 2), GFP_KERNEL);
...
if (len) {
...
} else {
memset(user, '\0', 2);
}

While Clang works on this bug[2], switch to using a direct assignment,
which avoids memset() entirely which both simplifies the code and silences
the false positive warning. (Making "len" size_t also silences the
warning, but the direct assignment seems better.)

Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://github.com/ClangBuiltLinux/linux/issues/1966 [1]
Link: https://github.com/llvm/llvm-project/issues/77813 [2]
Cc: Steve French <sfrench@samba.org>
Cc: Paulo Alcantara <pc@manguebit.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Shyam Prasad N <sprasad@microsoft.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: <linux-cifs@vger.kernel.org>
Cc: <llvm@lists.linux.dev>
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20240123234731.work.358-kees@kernel.org
---
fs/smb/client/cifsencrypt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
index ef4c2e3c9fa6..6322f0f68a17 100644
--- a/fs/smb/client/cifsencrypt.c
+++ b/fs/smb/client/cifsencrypt.c
@@ -572,7 +572,7 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
len = cifs_strtoUTF16(user, ses->user_name, len, nls_cp);
UniStrupr(user);
} else {
- memset(user, '\0', 2);
+ *(u16 *)user = 0;
}

rc = crypto_shash_update(ses->server->secmech.hmacmd5,
--
2.34.1

1 change: 1 addition & 0 deletions patches/6.6/series
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
6a4e59eeedc3018cb57722eecfcbb49431aeb05f.patch
20240123_keescook_smb_work_around_clang___bdos_type_confusion.patch
20240123_nathan_modpost_add_ltext_and_ltext_to_text_sections.patch
20240123_nathan_um_fix_adding_no_pie_for_clang.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
From git@z Thu Jan 1 00:00:00 1970
Subject: [PATCH] smb: Work around Clang __bdos() type confusion
From: Kees Cook <keescook@chromium.org>
Date: Tue, 23 Jan 2024 15:47:34 -0800
Message-Id: <20240123234731.work.358-kees@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit

Recent versions of Clang gets confused about the possible size of the
"user" allocation, and CONFIG_FORTIFY_SOURCE ends up emitting a
warning[1]:

repro.c:126:4: warning: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
126 | __write_overflow_field(p_size_field, size);
| ^

for this memset():

int len;
__le16 *user;
...
len = ses->user_name ? strlen(ses->user_name) : 0;
user = kmalloc(2 + (len * 2), GFP_KERNEL);
...
if (len) {
...
} else {
memset(user, '\0', 2);
}

While Clang works on this bug[2], switch to using a direct assignment,
which avoids memset() entirely which both simplifies the code and silences
the false positive warning. (Making "len" size_t also silences the
warning, but the direct assignment seems better.)

Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://github.com/ClangBuiltLinux/linux/issues/1966 [1]
Link: https://github.com/llvm/llvm-project/issues/77813 [2]
Cc: Steve French <sfrench@samba.org>
Cc: Paulo Alcantara <pc@manguebit.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Shyam Prasad N <sprasad@microsoft.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: <linux-cifs@vger.kernel.org>
Cc: <llvm@lists.linux.dev>
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20240123234731.work.358-kees@kernel.org
---
fs/smb/client/cifsencrypt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
index ef4c2e3c9fa6..6322f0f68a17 100644
--- a/fs/smb/client/cifsencrypt.c
+++ b/fs/smb/client/cifsencrypt.c
@@ -572,7 +572,7 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
len = cifs_strtoUTF16(user, ses->user_name, len, nls_cp);
UniStrupr(user);
} else {
- memset(user, '\0', 2);
+ *(u16 *)user = 0;
}

rc = crypto_shash_update(ses->server->secmech.hmacmd5,
--
2.34.1

1 change: 1 addition & 0 deletions patches/arm64-fixes/series
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
20240123_keescook_smb_work_around_clang___bdos_type_confusion.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
From git@z Thu Jan 1 00:00:00 1970
Subject: [PATCH] smb: Work around Clang __bdos() type confusion
From: Kees Cook <keescook@chromium.org>
Date: Tue, 23 Jan 2024 15:47:34 -0800
Message-Id: <20240123234731.work.358-kees@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit

Recent versions of Clang gets confused about the possible size of the
"user" allocation, and CONFIG_FORTIFY_SOURCE ends up emitting a
warning[1]:

repro.c:126:4: warning: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
126 | __write_overflow_field(p_size_field, size);
| ^

for this memset():

int len;
__le16 *user;
...
len = ses->user_name ? strlen(ses->user_name) : 0;
user = kmalloc(2 + (len * 2), GFP_KERNEL);
...
if (len) {
...
} else {
memset(user, '\0', 2);
}

While Clang works on this bug[2], switch to using a direct assignment,
which avoids memset() entirely which both simplifies the code and silences
the false positive warning. (Making "len" size_t also silences the
warning, but the direct assignment seems better.)

Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://github.com/ClangBuiltLinux/linux/issues/1966 [1]
Link: https://github.com/llvm/llvm-project/issues/77813 [2]
Cc: Steve French <sfrench@samba.org>
Cc: Paulo Alcantara <pc@manguebit.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Shyam Prasad N <sprasad@microsoft.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: <linux-cifs@vger.kernel.org>
Cc: <llvm@lists.linux.dev>
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20240123234731.work.358-kees@kernel.org
---
fs/smb/client/cifsencrypt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
index ef4c2e3c9fa6..6322f0f68a17 100644
--- a/fs/smb/client/cifsencrypt.c
+++ b/fs/smb/client/cifsencrypt.c
@@ -572,7 +572,7 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
len = cifs_strtoUTF16(user, ses->user_name, len, nls_cp);
UniStrupr(user);
} else {
- memset(user, '\0', 2);
+ *(u16 *)user = 0;
}

rc = crypto_shash_update(ses->server->secmech.hmacmd5,
--
2.34.1

1 change: 1 addition & 0 deletions patches/arm64/series
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
20240123_keescook_smb_work_around_clang___bdos_type_confusion.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
From git@z Thu Jan 1 00:00:00 1970
Subject: [PATCH] smb: Work around Clang __bdos() type confusion
From: Kees Cook <keescook@chromium.org>
Date: Tue, 23 Jan 2024 15:47:34 -0800
Message-Id: <20240123234731.work.358-kees@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit

Recent versions of Clang gets confused about the possible size of the
"user" allocation, and CONFIG_FORTIFY_SOURCE ends up emitting a
warning[1]:

repro.c:126:4: warning: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
126 | __write_overflow_field(p_size_field, size);
| ^

for this memset():

int len;
__le16 *user;
...
len = ses->user_name ? strlen(ses->user_name) : 0;
user = kmalloc(2 + (len * 2), GFP_KERNEL);
...
if (len) {
...
} else {
memset(user, '\0', 2);
}

While Clang works on this bug[2], switch to using a direct assignment,
which avoids memset() entirely which both simplifies the code and silences
the false positive warning. (Making "len" size_t also silences the
warning, but the direct assignment seems better.)

Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://github.com/ClangBuiltLinux/linux/issues/1966 [1]
Link: https://github.com/llvm/llvm-project/issues/77813 [2]
Cc: Steve French <sfrench@samba.org>
Cc: Paulo Alcantara <pc@manguebit.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Shyam Prasad N <sprasad@microsoft.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: <linux-cifs@vger.kernel.org>
Cc: <llvm@lists.linux.dev>
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20240123234731.work.358-kees@kernel.org
---
fs/smb/client/cifsencrypt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
index ef4c2e3c9fa6..6322f0f68a17 100644
--- a/fs/smb/client/cifsencrypt.c
+++ b/fs/smb/client/cifsencrypt.c
@@ -572,7 +572,7 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
len = cifs_strtoUTF16(user, ses->user_name, len, nls_cp);
UniStrupr(user);
} else {
- memset(user, '\0', 2);
+ *(u16 *)user = 0;
}

rc = crypto_shash_update(ses->server->secmech.hmacmd5,
--
2.34.1

1 change: 1 addition & 0 deletions patches/mainline/series
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
20240123_keescook_smb_work_around_clang___bdos_type_confusion.patch
20240123_nathan_modpost_add_ltext_and_ltext_to_text_sections.patch
20240123_nathan_um_fix_adding_no_pie_for_clang.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
From git@z Thu Jan 1 00:00:00 1970
Subject: [PATCH] smb: Work around Clang __bdos() type confusion
From: Kees Cook <keescook@chromium.org>
Date: Tue, 23 Jan 2024 15:47:34 -0800
Message-Id: <20240123234731.work.358-kees@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit

Recent versions of Clang gets confused about the possible size of the
"user" allocation, and CONFIG_FORTIFY_SOURCE ends up emitting a
warning[1]:

repro.c:126:4: warning: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
126 | __write_overflow_field(p_size_field, size);
| ^

for this memset():

int len;
__le16 *user;
...
len = ses->user_name ? strlen(ses->user_name) : 0;
user = kmalloc(2 + (len * 2), GFP_KERNEL);
...
if (len) {
...
} else {
memset(user, '\0', 2);
}

While Clang works on this bug[2], switch to using a direct assignment,
which avoids memset() entirely which both simplifies the code and silences
the false positive warning. (Making "len" size_t also silences the
warning, but the direct assignment seems better.)

Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://github.com/ClangBuiltLinux/linux/issues/1966 [1]
Link: https://github.com/llvm/llvm-project/issues/77813 [2]
Cc: Steve French <sfrench@samba.org>
Cc: Paulo Alcantara <pc@manguebit.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Shyam Prasad N <sprasad@microsoft.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: <linux-cifs@vger.kernel.org>
Cc: <llvm@lists.linux.dev>
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20240123234731.work.358-kees@kernel.org
---
fs/smb/client/cifsencrypt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
index ef4c2e3c9fa6..6322f0f68a17 100644
--- a/fs/smb/client/cifsencrypt.c
+++ b/fs/smb/client/cifsencrypt.c
@@ -572,7 +572,7 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
len = cifs_strtoUTF16(user, ses->user_name, len, nls_cp);
UniStrupr(user);
} else {
- memset(user, '\0', 2);
+ *(u16 *)user = 0;
}

rc = crypto_shash_update(ses->server->secmech.hmacmd5,
--
2.34.1

1 change: 1 addition & 0 deletions patches/stable/series
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
20240123_keescook_smb_work_around_clang___bdos_type_confusion.patch
20240123_nathan_modpost_add_ltext_and_ltext_to_text_sections.patch
20240123_nathan_um_fix_adding_no_pie_for_clang.patch
Loading

0 comments on commit dcd26b9

Please sign in to comment.