| Message ID | 20221001225324.21771-1-atteh.mailbox@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ksmbd: validate share name from share config response | expand |
On 10/1/2022 6:53 PM, Atte Heikkilä wrote: > Share config response may contain the share name without casefolding as > it is known to the user space daemon. When it is present, casefold and > compare it to the share name the share config request was made with. If > they differ, we have a share config which is incompatible with the way > share config caching is done. This is the case when CONFIG_UNICODE is > not set, the share name contains non-ASCII characters, and those non- > ASCII characters do not match those in the share name known to user > space. In other words, when CONFIG_UNICODE is not set, UTF-8 share > names now work but are only case-insensitive in the ASCII range. > > Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com> > --- > v3: > - removed initial strcmp() check since it could only save a call to > ksmbd_casefold_sharename() for matching ASCII-only share names > > v2: > - no changes were made > > fs/ksmbd/ksmbd_netlink.h | 3 ++- > fs/ksmbd/mgmt/share_config.c | 20 +++++++++++++++++--- > fs/ksmbd/mgmt/share_config.h | 4 +++- > fs/ksmbd/mgmt/tree_connect.c | 4 ++-- > fs/ksmbd/misc.c | 4 ++-- > fs/ksmbd/misc.h | 1 + > 6 files changed, 27 insertions(+), 9 deletions(-) > > diff --git a/fs/ksmbd/ksmbd_netlink.h b/fs/ksmbd/ksmbd_netlink.h > index e0cbcfa98c7e..ff07c67f4565 100644 > --- a/fs/ksmbd/ksmbd_netlink.h > +++ b/fs/ksmbd/ksmbd_netlink.h > @@ -163,7 +163,8 @@ struct ksmbd_share_config_response { > __u16 force_directory_mode; > __u16 force_uid; > __u16 force_gid; > - __u32 reserved[128]; /* Reserved room */ > + __s8 share_name[KSMBD_REQ_MAX_SHARE_NAME]; > + __u32 reserved[112]; /* Reserved room */ Because this is a protocol/abi definition, it may be good practice to code as __u32 reserved[128 - KSMBD_REQ_MAX_SHARE_NAME]; just in case the sharename constant changes in future. > __u32 veto_list_sz; > __s8 ____payload[]; > }; > diff --git a/fs/ksmbd/mgmt/share_config.c b/fs/ksmbd/mgmt/share_config.c > index 5d039704c23c..c62e87b67247 100644 > --- a/fs/ksmbd/mgmt/share_config.c > +++ b/fs/ksmbd/mgmt/share_config.c > @@ -16,6 +16,7 @@ > #include "user_config.h" > #include "user_session.h" > #include "../transport_ipc.h" > +#include "../misc.h" > > #define SHARE_HASH_BITS 3 > static DEFINE_HASHTABLE(shares_table, SHARE_HASH_BITS); > @@ -119,7 +120,8 @@ static int parse_veto_list(struct ksmbd_share_config *share, > return 0; > } > > -static struct ksmbd_share_config *share_config_request(const char *name) > +static struct ksmbd_share_config *share_config_request(struct unicode_map *um, > + const char *name) > { > struct ksmbd_share_config_response *resp; > struct ksmbd_share_config *share = NULL; > @@ -133,6 +135,17 @@ static struct ksmbd_share_config *share_config_request(const char *name) > if (resp->flags == KSMBD_SHARE_FLAG_INVALID) > goto out; > > + if (*resp->share_name) { > + char *cf_resp_name; > + bool equal; > + > + cf_resp_name = ksmbd_casefold_sharename(um, resp->share_name); > + equal = cf_resp_name && !strcmp(cf_resp_name, name); This is a little bit hard to parse. So, if the casefolding returns a non-null string, and the result does not equal the original name? Maybe because it's evaluating a pointer as a bool, and using "!" to evaluate the strcpy result is the confusing part, to me. equal = cf_resp_name != NULL && strcmp(cf_resp_name, name) 0= 0; Also, strcmp() gets a lot of attention in the kernel these days. Is this guaranteed safe? > + kfree(cf_resp_name); > + if (!equal) > + goto out; > + } > + > share = kzalloc(sizeof(struct ksmbd_share_config), GFP_KERNEL); > if (!share) > goto out; > @@ -190,7 +203,8 @@ static struct ksmbd_share_config *share_config_request(const char *name) > return share; > } > > -struct ksmbd_share_config *ksmbd_share_config_get(const char *name) > +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map *um, > + const char *name) > { > struct ksmbd_share_config *share; > > @@ -202,7 +216,7 @@ struct ksmbd_share_config *ksmbd_share_config_get(const char *name) > > if (share) > return share; > - return share_config_request(name); > + return share_config_request(um, name); > } > > bool ksmbd_share_veto_filename(struct ksmbd_share_config *share, > diff --git a/fs/ksmbd/mgmt/share_config.h b/fs/ksmbd/mgmt/share_config.h > index 7f7e89ecfe61..3fd338293942 100644 > --- a/fs/ksmbd/mgmt/share_config.h > +++ b/fs/ksmbd/mgmt/share_config.h > @@ -9,6 +9,7 @@ > #include <linux/workqueue.h> > #include <linux/hashtable.h> > #include <linux/path.h> > +#include <linux/unicode.h> > > struct ksmbd_share_config { > char *name; > @@ -74,7 +75,8 @@ static inline void ksmbd_share_config_put(struct ksmbd_share_config *share) > __ksmbd_share_config_put(share); > } > > -struct ksmbd_share_config *ksmbd_share_config_get(const char *name); > +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map *um, > + const char *name); > bool ksmbd_share_veto_filename(struct ksmbd_share_config *share, > const char *filename); > #endif /* __SHARE_CONFIG_MANAGEMENT_H__ */ > diff --git a/fs/ksmbd/mgmt/tree_connect.c b/fs/ksmbd/mgmt/tree_connect.c > index 867c0286b901..8ce17b3fb8da 100644 > --- a/fs/ksmbd/mgmt/tree_connect.c > +++ b/fs/ksmbd/mgmt/tree_connect.c > @@ -26,7 +26,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess, > struct sockaddr *peer_addr; > int ret; > > - sc = ksmbd_share_config_get(share_name); > + sc = ksmbd_share_config_get(conn->um, share_name); > if (!sc) Another pointer boolean evaluation, but I guess this one is existing. > return status; > > @@ -61,7 +61,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess, > struct ksmbd_share_config *new_sc; > > ksmbd_share_config_del(sc); > - new_sc = ksmbd_share_config_get(share_name); > + new_sc = ksmbd_share_config_get(conn->um, share_name); > if (!new_sc) { Ditto > pr_err("Failed to update stale share config\n"); > status.ret = -ESTALE; > diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c > index 28459b1efaa8..9e8afaa686e3 100644 > --- a/fs/ksmbd/misc.c > +++ b/fs/ksmbd/misc.c > @@ -227,7 +227,7 @@ void ksmbd_conv_path_to_windows(char *path) > strreplace(path, '/', '\\'); > } > > -static char *casefold_sharename(struct unicode_map *um, const char *name) > +char *ksmbd_casefold_sharename(struct unicode_map *um, const char *name) > { > char *cf_name; > int cf_len; > @@ -273,7 +273,7 @@ char *ksmbd_extract_sharename(struct unicode_map *um, const char *treename) > name = (pos + 1); > > /* caller has to free the memory */ > - return casefold_sharename(um, name); > + return ksmbd_casefold_sharename(um, name); > } > > /** > diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h > index cc72f4e6baf2..1facfcd21200 100644 > --- a/fs/ksmbd/misc.h > +++ b/fs/ksmbd/misc.h > @@ -20,6 +20,7 @@ int get_nlink(struct kstat *st); > void ksmbd_conv_path_to_unix(char *path); > void ksmbd_strip_last_slash(char *path); > void ksmbd_conv_path_to_windows(char *path); > +char *ksmbd_casefold_sharename(struct unicode_map *um, const char *name); > char *ksmbd_extract_sharename(struct unicode_map *um, const char *treename); > char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name); >
On Sat, 1 Oct 2022 19:20:52 -0400, Tom Talpey wrote: > On 10/1/2022 6:53 PM, Atte Heikkilä wrote: >> Share config response may contain the share name without casefolding as >> it is known to the user space daemon. When it is present, casefold and >> compare it to the share name the share config request was made with. If >> they differ, we have a share config which is incompatible with the way >> share config caching is done. This is the case when CONFIG_UNICODE is >> not set, the share name contains non-ASCII characters, and those non- >> ASCII characters do not match those in the share name known to user >> space. In other words, when CONFIG_UNICODE is not set, UTF-8 share >> names now work but are only case-insensitive in the ASCII range. >> >> Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com> >> --- >> v3: >> - removed initial strcmp() check since it could only save a call to >> ksmbd_casefold_sharename() for matching ASCII-only share names >> >> v2: >> - no changes were made >> >> fs/ksmbd/ksmbd_netlink.h | 3 ++- >> fs/ksmbd/mgmt/share_config.c | 20 +++++++++++++++++--- >> fs/ksmbd/mgmt/share_config.h | 4 +++- >> fs/ksmbd/mgmt/tree_connect.c | 4 ++-- >> fs/ksmbd/misc.c | 4 ++-- >> fs/ksmbd/misc.h | 1 + >> 6 files changed, 27 insertions(+), 9 deletions(-) >> >> diff --git a/fs/ksmbd/ksmbd_netlink.h b/fs/ksmbd/ksmbd_netlink.h >> index e0cbcfa98c7e..ff07c67f4565 100644 >> --- a/fs/ksmbd/ksmbd_netlink.h >> +++ b/fs/ksmbd/ksmbd_netlink.h >> @@ -163,7 +163,8 @@ struct ksmbd_share_config_response { >> __u16 force_directory_mode; >> __u16 force_uid; >> __u16 force_gid; >> - __u32 reserved[128]; /* Reserved room */ >> + __s8 share_name[KSMBD_REQ_MAX_SHARE_NAME]; >> + __u32 reserved[112]; /* Reserved room */ > > Because this is a protocol/abi definition, it may be good practice > to code as > __u32 reserved[128 - KSMBD_REQ_MAX_SHARE_NAME]; > just in case the sharename constant changes in future. > You are right. That would probably be better. >> __u32 veto_list_sz; >> __s8 ____payload[]; >> }; >> diff --git a/fs/ksmbd/mgmt/share_config.c b/fs/ksmbd/mgmt/share_config.c >> index 5d039704c23c..c62e87b67247 100644 >> --- a/fs/ksmbd/mgmt/share_config.c >> +++ b/fs/ksmbd/mgmt/share_config.c >> @@ -16,6 +16,7 @@ >> #include "user_config.h" >> #include "user_session.h" >> #include "../transport_ipc.h" >> +#include "../misc.h" >> >> #define SHARE_HASH_BITS 3 >> static DEFINE_HASHTABLE(shares_table, SHARE_HASH_BITS); >> @@ -119,7 +120,8 @@ static int parse_veto_list(struct ksmbd_share_config *share, >> return 0; >> } >> >> -static struct ksmbd_share_config *share_config_request(const char *name) >> +static struct ksmbd_share_config *share_config_request(struct unicode_map *um, >> + const char *name) >> { >> struct ksmbd_share_config_response *resp; >> struct ksmbd_share_config *share = NULL; >> @@ -133,6 +135,17 @@ static struct ksmbd_share_config *share_config_request(const char *name) >> if (resp->flags == KSMBD_SHARE_FLAG_INVALID) >> goto out; >> >> + if (*resp->share_name) { >> + char *cf_resp_name; >> + bool equal; >> + >> + cf_resp_name = ksmbd_casefold_sharename(um, resp->share_name); >> + equal = cf_resp_name && !strcmp(cf_resp_name, name); > > This is a little bit hard to parse. So, if the casefolding returns > a non-null string, and the result does not equal the original name? > Maybe because it's evaluating a pointer as a bool, and using "!" to > evaluate the strcpy result is the confusing part, to me. > > equal = cf_resp_name != NULL && strcmp(cf_resp_name, name) 0= 0; > It should be `!IS_ERR(cf_resp_name) && !strcmp(cf_resp_name, name)'. The share name in the share config response is the actual share name which is known to user space. It is then casefolded according to the kernel's capabilities, which is either utf8_casefold() or tolower(). If it differs from the casefolded form of the share name used by the client, then the client-used share name is incompatible with the way the kernel does share config caching. > Also, strcmp() gets a lot of attention in the kernel these days. Is > this guaranteed safe? Both are always null-terminated if that is what you are asking. Tom, thank you very much for taking a look. I would not have noticed my careless mistake otherwise. > >> + kfree(cf_resp_name); >> + if (!equal) >> + goto out; >> + } >> + >> share = kzalloc(sizeof(struct ksmbd_share_config), GFP_KERNEL); >> if (!share) >> goto out; >> @@ -190,7 +203,8 @@ static struct ksmbd_share_config *share_config_request(const char *name) >> return share; >> } >> >> -struct ksmbd_share_config *ksmbd_share_config_get(const char *name) >> +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map *um, >> + const char *name) >> { >> struct ksmbd_share_config *share; >> >> @@ -202,7 +216,7 @@ struct ksmbd_share_config *ksmbd_share_config_get(const char *name) >> >> if (share) >> return share; >> - return share_config_request(name); >> + return share_config_request(um, name); >> } >> >> bool ksmbd_share_veto_filename(struct ksmbd_share_config *share, >> diff --git a/fs/ksmbd/mgmt/share_config.h b/fs/ksmbd/mgmt/share_config.h >> index 7f7e89ecfe61..3fd338293942 100644 >> --- a/fs/ksmbd/mgmt/share_config.h >> +++ b/fs/ksmbd/mgmt/share_config.h >> @@ -9,6 +9,7 @@ >> #include <linux/workqueue.h> >> #include <linux/hashtable.h> >> #include <linux/path.h> >> +#include <linux/unicode.h> >> >> struct ksmbd_share_config { >> char *name; >> @@ -74,7 +75,8 @@ static inline void ksmbd_share_config_put(struct ksmbd_share_config *share) >> __ksmbd_share_config_put(share); >> } >> >> -struct ksmbd_share_config *ksmbd_share_config_get(const char *name); >> +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map *um, >> + const char *name); >> bool ksmbd_share_veto_filename(struct ksmbd_share_config *share, >> const char *filename); >> #endif /* __SHARE_CONFIG_MANAGEMENT_H__ */ >> diff --git a/fs/ksmbd/mgmt/tree_connect.c b/fs/ksmbd/mgmt/tree_connect.c >> index 867c0286b901..8ce17b3fb8da 100644 >> --- a/fs/ksmbd/mgmt/tree_connect.c >> +++ b/fs/ksmbd/mgmt/tree_connect.c >> @@ -26,7 +26,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess, >> struct sockaddr *peer_addr; >> int ret; >> >> - sc = ksmbd_share_config_get(share_name); >> + sc = ksmbd_share_config_get(conn->um, share_name); >> if (!sc) > > Another pointer boolean evaluation, but I guess this one is existing. > >> return status; >> >> @@ -61,7 +61,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess, >> struct ksmbd_share_config *new_sc; >> >> ksmbd_share_config_del(sc); >> - new_sc = ksmbd_share_config_get(share_name); >> + new_sc = ksmbd_share_config_get(conn->um, share_name); >> if (!new_sc) { > > Ditto >> pr_err("Failed to update stale share config\n"); >> status.ret = -ESTALE; >> diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c >> index 28459b1efaa8..9e8afaa686e3 100644 >> --- a/fs/ksmbd/misc.c >> +++ b/fs/ksmbd/misc.c >> @@ -227,7 +227,7 @@ void ksmbd_conv_path_to_windows(char *path) >> strreplace(path, '/', '\\'); >> } >> >> -static char *casefold_sharename(struct unicode_map *um, const char *name) >> +char *ksmbd_casefold_sharename(struct unicode_map *um, const char *name) >> { >> char *cf_name; >> int cf_len; >> @@ -273,7 +273,7 @@ char *ksmbd_extract_sharename(struct unicode_map *um, const char *treename) >> name = (pos + 1); >> >> /* caller has to free the memory */ >> - return casefold_sharename(um, name); >> + return ksmbd_casefold_sharename(um, name); >> } >> >> /** >> diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h >> index cc72f4e6baf2..1facfcd21200 100644 >> --- a/fs/ksmbd/misc.h >> +++ b/fs/ksmbd/misc.h >> @@ -20,6 +20,7 @@ int get_nlink(struct kstat *st); >> void ksmbd_conv_path_to_unix(char *path); >> void ksmbd_strip_last_slash(char *path); >> void ksmbd_conv_path_to_windows(char *path); >> +char *ksmbd_casefold_sharename(struct unicode_map *um, const char *name); >> char *ksmbd_extract_sharename(struct unicode_map *um, const char *treename); >> char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name); >> >
On 10/1/2022 8:25 PM, Atte Heikkilä wrote: > On Sat, 1 Oct 2022 19:20:52 -0400, Tom Talpey wrote: >> On 10/1/2022 6:53 PM, Atte Heikkilä wrote: >>> Share config response may contain the share name without casefolding as >>> it is known to the user space daemon. When it is present, casefold and >>> compare it to the share name the share config request was made with. If >>> they differ, we have a share config which is incompatible with the way >>> share config caching is done. This is the case when CONFIG_UNICODE is >>> not set, the share name contains non-ASCII characters, and those non- >>> ASCII characters do not match those in the share name known to user >>> space. In other words, when CONFIG_UNICODE is not set, UTF-8 share >>> names now work but are only case-insensitive in the ASCII range. >>> >>> Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com> >>> --- >>> v3: >>> - removed initial strcmp() check since it could only save a call to >>> ksmbd_casefold_sharename() for matching ASCII-only share names >>> >>> v2: >>> - no changes were made >>> >>> fs/ksmbd/ksmbd_netlink.h | 3 ++- >>> fs/ksmbd/mgmt/share_config.c | 20 +++++++++++++++++--- >>> fs/ksmbd/mgmt/share_config.h | 4 +++- >>> fs/ksmbd/mgmt/tree_connect.c | 4 ++-- >>> fs/ksmbd/misc.c | 4 ++-- >>> fs/ksmbd/misc.h | 1 + >>> 6 files changed, 27 insertions(+), 9 deletions(-) >>> >>> diff --git a/fs/ksmbd/ksmbd_netlink.h b/fs/ksmbd/ksmbd_netlink.h >>> index e0cbcfa98c7e..ff07c67f4565 100644 >>> --- a/fs/ksmbd/ksmbd_netlink.h >>> +++ b/fs/ksmbd/ksmbd_netlink.h >>> @@ -163,7 +163,8 @@ struct ksmbd_share_config_response { >>> __u16 force_directory_mode; >>> __u16 force_uid; >>> __u16 force_gid; >>> - __u32 reserved[128]; /* Reserved room */ >>> + __s8 share_name[KSMBD_REQ_MAX_SHARE_NAME]; >>> + __u32 reserved[112]; /* Reserved room */ >> >> Because this is a protocol/abi definition, it may be good practice >> to code as >> __u32 reserved[128 - KSMBD_REQ_MAX_SHARE_NAME]; >> just in case the sharename constant changes in future. >> > > You are right. That would probably be better. > >>> __u32 veto_list_sz; >>> __s8 ____payload[]; >>> }; >>> diff --git a/fs/ksmbd/mgmt/share_config.c b/fs/ksmbd/mgmt/share_config.c >>> index 5d039704c23c..c62e87b67247 100644 >>> --- a/fs/ksmbd/mgmt/share_config.c >>> +++ b/fs/ksmbd/mgmt/share_config.c >>> @@ -16,6 +16,7 @@ >>> #include "user_config.h" >>> #include "user_session.h" >>> #include "../transport_ipc.h" >>> +#include "../misc.h" >>> #define SHARE_HASH_BITS 3 >>> static DEFINE_HASHTABLE(shares_table, SHARE_HASH_BITS); >>> @@ -119,7 +120,8 @@ static int parse_veto_list(struct >>> ksmbd_share_config *share, >>> return 0; >>> } >>> -static struct ksmbd_share_config *share_config_request(const char >>> *name) >>> +static struct ksmbd_share_config *share_config_request(struct >>> unicode_map *um, >>> + const char *name) >>> { >>> struct ksmbd_share_config_response *resp; >>> struct ksmbd_share_config *share = NULL; >>> @@ -133,6 +135,17 @@ static struct ksmbd_share_config >>> *share_config_request(const char *name) >>> if (resp->flags == KSMBD_SHARE_FLAG_INVALID) >>> goto out; >>> + if (*resp->share_name) { >>> + char *cf_resp_name; >>> + bool equal; >>> + >>> + cf_resp_name = ksmbd_casefold_sharename(um, resp->share_name); >>> + equal = cf_resp_name && !strcmp(cf_resp_name, name); >> >> This is a little bit hard to parse. So, if the casefolding returns >> a non-null string, and the result does not equal the original name? >> Maybe because it's evaluating a pointer as a bool, and using "!" to >> evaluate the strcpy result is the confusing part, to me. >> >> equal = cf_resp_name != NULL && strcmp(cf_resp_name, name) 0= 0; >> > > It should be `!IS_ERR(cf_resp_name) && !strcmp(cf_resp_name, name)'. Aha - ok that's better (but I still have to pause after seeing "!strcmp") > The share name in the share config response is the actual share name > which is known to user space. It is then casefolded according to the > kernel's capabilities, which is either utf8_casefold() or tolower(). > If it differs from the casefolded form of the share name used by the > client, then the client-used share name is incompatible with the way > the kernel does share config caching. > >> Also, strcmp() gets a lot of attention in the kernel these days. Is >> this guaranteed safe? > > Both are always null-terminated if that is what you are asking. Ok, but are they guaranteed to be ascii? Isn't strcmp doing an octet based comparison? Ned Pyle has a hilarious demo where he sets a servername to the Bacon emoji, and a share name to Eggs-emoji, or something. I'm not sure if he creates a Fork-emoji file, but it's something like that. He then proceeds to show how it all works with Windows. I just don't want to see us running over the end of crazy unterminated or Unicode expansion strings. > Tom, thank you very much for taking a look. I would not have noticed my > careless mistake otherwise. > >> >>> + kfree(cf_resp_name); >>> + if (!equal) >>> + goto out; >>> + } >>> + >>> share = kzalloc(sizeof(struct ksmbd_share_config), GFP_KERNEL); >>> if (!share) >>> goto out; >>> @@ -190,7 +203,8 @@ static struct ksmbd_share_config >>> *share_config_request(const char *name) >>> return share; >>> } >>> -struct ksmbd_share_config *ksmbd_share_config_get(const char *name) >>> +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map >>> *um, >>> + const char *name) >>> { >>> struct ksmbd_share_config *share; >>> @@ -202,7 +216,7 @@ struct ksmbd_share_config >>> *ksmbd_share_config_get(const char *name) >>> if (share) >>> return share; >>> - return share_config_request(name); >>> + return share_config_request(um, name); >>> } >>> bool ksmbd_share_veto_filename(struct ksmbd_share_config *share, >>> diff --git a/fs/ksmbd/mgmt/share_config.h b/fs/ksmbd/mgmt/share_config.h >>> index 7f7e89ecfe61..3fd338293942 100644 >>> --- a/fs/ksmbd/mgmt/share_config.h >>> +++ b/fs/ksmbd/mgmt/share_config.h >>> @@ -9,6 +9,7 @@ >>> #include <linux/workqueue.h> >>> #include <linux/hashtable.h> >>> #include <linux/path.h> >>> +#include <linux/unicode.h> >>> struct ksmbd_share_config { >>> char *name; >>> @@ -74,7 +75,8 @@ static inline void ksmbd_share_config_put(struct >>> ksmbd_share_config *share) >>> __ksmbd_share_config_put(share); >>> } >>> -struct ksmbd_share_config *ksmbd_share_config_get(const char *name); >>> +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map >>> *um, >>> + const char *name); >>> bool ksmbd_share_veto_filename(struct ksmbd_share_config *share, >>> const char *filename); >>> #endif /* __SHARE_CONFIG_MANAGEMENT_H__ */ >>> diff --git a/fs/ksmbd/mgmt/tree_connect.c b/fs/ksmbd/mgmt/tree_connect.c >>> index 867c0286b901..8ce17b3fb8da 100644 >>> --- a/fs/ksmbd/mgmt/tree_connect.c >>> +++ b/fs/ksmbd/mgmt/tree_connect.c >>> @@ -26,7 +26,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, >>> struct ksmbd_session *sess, >>> struct sockaddr *peer_addr; >>> int ret; >>> - sc = ksmbd_share_config_get(share_name); >>> + sc = ksmbd_share_config_get(conn->um, share_name); >>> if (!sc) >> >> Another pointer boolean evaluation, but I guess this one is existing. >> >>> return status; >>> @@ -61,7 +61,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, >>> struct ksmbd_session *sess, >>> struct ksmbd_share_config *new_sc; >>> ksmbd_share_config_del(sc); >>> - new_sc = ksmbd_share_config_get(share_name); >>> + new_sc = ksmbd_share_config_get(conn->um, share_name); >>> if (!new_sc) { >> >> Ditto >>> pr_err("Failed to update stale share config\n"); >>> status.ret = -ESTALE; >>> diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c >>> index 28459b1efaa8..9e8afaa686e3 100644 >>> --- a/fs/ksmbd/misc.c >>> +++ b/fs/ksmbd/misc.c >>> @@ -227,7 +227,7 @@ void ksmbd_conv_path_to_windows(char *path) >>> strreplace(path, '/', '\\'); >>> } >>> -static char *casefold_sharename(struct unicode_map *um, const char >>> *name) >>> +char *ksmbd_casefold_sharename(struct unicode_map *um, const char >>> *name) >>> { >>> char *cf_name; >>> int cf_len; >>> @@ -273,7 +273,7 @@ char *ksmbd_extract_sharename(struct unicode_map >>> *um, const char *treename) >>> name = (pos + 1); >>> /* caller has to free the memory */ >>> - return casefold_sharename(um, name); >>> + return ksmbd_casefold_sharename(um, name); >>> } >>> /** >>> diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h >>> index cc72f4e6baf2..1facfcd21200 100644 >>> --- a/fs/ksmbd/misc.h >>> +++ b/fs/ksmbd/misc.h >>> @@ -20,6 +20,7 @@ int get_nlink(struct kstat *st); >>> void ksmbd_conv_path_to_unix(char *path); >>> void ksmbd_strip_last_slash(char *path); >>> void ksmbd_conv_path_to_windows(char *path); >>> +char *ksmbd_casefold_sharename(struct unicode_map *um, const char >>> *name); >>> char *ksmbd_extract_sharename(struct unicode_map *um, const char >>> *treename); >>> char *convert_to_unix_name(struct ksmbd_share_config *share, const >>> char *name); >> >
On Sat, 1 Oct 2022 20:38:06 -0400, Tom Talpey wrote: > On 10/1/2022 8:25 PM, Atte Heikkilä wrote: >> On Sat, 1 Oct 2022 19:20:52 -0400, Tom Talpey wrote: >>> On 10/1/2022 6:53 PM, Atte Heikkilä wrote: >>>> Share config response may contain the share name without casefolding as >>>> it is known to the user space daemon. When it is present, casefold and >>>> compare it to the share name the share config request was made with. If >>>> they differ, we have a share config which is incompatible with the way >>>> share config caching is done. This is the case when CONFIG_UNICODE is >>>> not set, the share name contains non-ASCII characters, and those non- >>>> ASCII characters do not match those in the share name known to user >>>> space. In other words, when CONFIG_UNICODE is not set, UTF-8 share >>>> names now work but are only case-insensitive in the ASCII range. >>>> >>>> Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com> >>>> --- >>>> v3: >>>> - removed initial strcmp() check since it could only save a call to >>>> ksmbd_casefold_sharename() for matching ASCII-only share names >>>> >>>> v2: >>>> - no changes were made >>>> >>>> fs/ksmbd/ksmbd_netlink.h | 3 ++- >>>> fs/ksmbd/mgmt/share_config.c | 20 +++++++++++++++++--- >>>> fs/ksmbd/mgmt/share_config.h | 4 +++- >>>> fs/ksmbd/mgmt/tree_connect.c | 4 ++-- >>>> fs/ksmbd/misc.c | 4 ++-- >>>> fs/ksmbd/misc.h | 1 + >>>> 6 files changed, 27 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/fs/ksmbd/ksmbd_netlink.h b/fs/ksmbd/ksmbd_netlink.h >>>> index e0cbcfa98c7e..ff07c67f4565 100644 >>>> --- a/fs/ksmbd/ksmbd_netlink.h >>>> +++ b/fs/ksmbd/ksmbd_netlink.h >>>> @@ -163,7 +163,8 @@ struct ksmbd_share_config_response { >>>> __u16 force_directory_mode; >>>> __u16 force_uid; >>>> __u16 force_gid; >>>> - __u32 reserved[128]; /* Reserved room */ >>>> + __s8 share_name[KSMBD_REQ_MAX_SHARE_NAME]; >>>> + __u32 reserved[112]; /* Reserved room */ >>> >>> Because this is a protocol/abi definition, it may be good practice >>> to code as >>> __u32 reserved[128 - KSMBD_REQ_MAX_SHARE_NAME]; >>> just in case the sharename constant changes in future. >>> >> >> You are right. That would probably be better. >> >>>> __u32 veto_list_sz; >>>> __s8 ____payload[]; >>>> }; >>>> diff --git a/fs/ksmbd/mgmt/share_config.c b/fs/ksmbd/mgmt/share_config.c >>>> index 5d039704c23c..c62e87b67247 100644 >>>> --- a/fs/ksmbd/mgmt/share_config.c >>>> +++ b/fs/ksmbd/mgmt/share_config.c >>>> @@ -16,6 +16,7 @@ >>>> #include "user_config.h" >>>> #include "user_session.h" >>>> #include "../transport_ipc.h" >>>> +#include "../misc.h" >>>> #define SHARE_HASH_BITS 3 >>>> static DEFINE_HASHTABLE(shares_table, SHARE_HASH_BITS); >>>> @@ -119,7 +120,8 @@ static int parse_veto_list(struct >>>> ksmbd_share_config *share, >>>> return 0; >>>> } >>>> -static struct ksmbd_share_config *share_config_request(const char >>>> *name) >>>> +static struct ksmbd_share_config *share_config_request(struct >>>> unicode_map *um, >>>> + const char *name) >>>> { >>>> struct ksmbd_share_config_response *resp; >>>> struct ksmbd_share_config *share = NULL; >>>> @@ -133,6 +135,17 @@ static struct ksmbd_share_config >>>> *share_config_request(const char *name) >>>> if (resp->flags == KSMBD_SHARE_FLAG_INVALID) >>>> goto out; >>>> + if (*resp->share_name) { >>>> + char *cf_resp_name; >>>> + bool equal; >>>> + >>>> + cf_resp_name = ksmbd_casefold_sharename(um, resp->share_name); >>>> + equal = cf_resp_name && !strcmp(cf_resp_name, name); >>> >>> This is a little bit hard to parse. So, if the casefolding returns >>> a non-null string, and the result does not equal the original name? >>> Maybe because it's evaluating a pointer as a bool, and using "!" to >>> evaluate the strcpy result is the confusing part, to me. >>> >>> equal = cf_resp_name != NULL && strcmp(cf_resp_name, name) 0= 0; >>> >> >> It should be `!IS_ERR(cf_resp_name) && !strcmp(cf_resp_name, name)'. > > Aha - ok that's better (but I still have to pause after seeing "!strcmp") > It looks like `!strcmp()' sees use over `strcmp() == 0' in ksmbd. >> The share name in the share config response is the actual share name >> which is known to user space. It is then casefolded according to the >> kernel's capabilities, which is either utf8_casefold() or tolower(). >> If it differs from the casefolded form of the share name used by the >> client, then the client-used share name is incompatible with the way >> the kernel does share config caching. >> >>> Also, strcmp() gets a lot of attention in the kernel these days. Is >>> this guaranteed safe? >> >> Both are always null-terminated if that is what you are asking. > > Ok, but are they guaranteed to be ascii? Isn't strcmp doing an octet > based comparison? > They are both casefolded UTF-8 or the lowercase ASCII subset of it. utf8_casefold() returns `nlen' only when the last written byte to the output `dest' buffer is the null terminator. Otherwise, when it returns -EINVAL, or when CONFIG_UNICODE is not set, ksmbd_casefold_sharename() falls back to tolower() guarded by isascii() so as to not touch the UTF-8-incompatible ISO8859-1 range. As I understand it, octet-wise comparison terminated by null byte is just fine here. > Ned Pyle has a hilarious demo where he sets a servername to the Bacon > emoji, and a share name to Eggs-emoji, or something. I'm not sure if > he creates a Fork-emoji file, but it's something like that. He then > proceeds to show how it all works with Windows. I just don't want to > see us running over the end of crazy unterminated or Unicode expansion > strings. > >> Tom, thank you very much for taking a look. I would not have noticed my >> careless mistake otherwise. >> >>> >>>> + kfree(cf_resp_name); >>>> + if (!equal) >>>> + goto out; >>>> + } >>>> + >>>> share = kzalloc(sizeof(struct ksmbd_share_config), GFP_KERNEL); >>>> if (!share) >>>> goto out; >>>> @@ -190,7 +203,8 @@ static struct ksmbd_share_config >>>> *share_config_request(const char *name) >>>> return share; >>>> } >>>> -struct ksmbd_share_config *ksmbd_share_config_get(const char *name) >>>> +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map >>>> *um, >>>> + const char *name) >>>> { >>>> struct ksmbd_share_config *share; >>>> @@ -202,7 +216,7 @@ struct ksmbd_share_config >>>> *ksmbd_share_config_get(const char *name) >>>> if (share) >>>> return share; >>>> - return share_config_request(name); >>>> + return share_config_request(um, name); >>>> } >>>> bool ksmbd_share_veto_filename(struct ksmbd_share_config *share, >>>> diff --git a/fs/ksmbd/mgmt/share_config.h b/fs/ksmbd/mgmt/share_config.h >>>> index 7f7e89ecfe61..3fd338293942 100644 >>>> --- a/fs/ksmbd/mgmt/share_config.h >>>> +++ b/fs/ksmbd/mgmt/share_config.h >>>> @@ -9,6 +9,7 @@ >>>> #include <linux/workqueue.h> >>>> #include <linux/hashtable.h> >>>> #include <linux/path.h> >>>> +#include <linux/unicode.h> >>>> struct ksmbd_share_config { >>>> char *name; >>>> @@ -74,7 +75,8 @@ static inline void ksmbd_share_config_put(struct >>>> ksmbd_share_config *share) >>>> __ksmbd_share_config_put(share); >>>> } >>>> -struct ksmbd_share_config *ksmbd_share_config_get(const char *name); >>>> +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map >>>> *um, >>>> + const char *name); >>>> bool ksmbd_share_veto_filename(struct ksmbd_share_config *share, >>>> const char *filename); >>>> #endif /* __SHARE_CONFIG_MANAGEMENT_H__ */ >>>> diff --git a/fs/ksmbd/mgmt/tree_connect.c b/fs/ksmbd/mgmt/tree_connect.c >>>> index 867c0286b901..8ce17b3fb8da 100644 >>>> --- a/fs/ksmbd/mgmt/tree_connect.c >>>> +++ b/fs/ksmbd/mgmt/tree_connect.c >>>> @@ -26,7 +26,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, >>>> struct ksmbd_session *sess, >>>> struct sockaddr *peer_addr; >>>> int ret; >>>> - sc = ksmbd_share_config_get(share_name); >>>> + sc = ksmbd_share_config_get(conn->um, share_name); >>>> if (!sc) >>> >>> Another pointer boolean evaluation, but I guess this one is existing. >>> >>>> return status; >>>> @@ -61,7 +61,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, >>>> struct ksmbd_session *sess, >>>> struct ksmbd_share_config *new_sc; >>>> ksmbd_share_config_del(sc); >>>> - new_sc = ksmbd_share_config_get(share_name); >>>> + new_sc = ksmbd_share_config_get(conn->um, share_name); >>>> if (!new_sc) { >>> >>> Ditto >>>> pr_err("Failed to update stale share config\n"); >>>> status.ret = -ESTALE; >>>> diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c >>>> index 28459b1efaa8..9e8afaa686e3 100644 >>>> --- a/fs/ksmbd/misc.c >>>> +++ b/fs/ksmbd/misc.c >>>> @@ -227,7 +227,7 @@ void ksmbd_conv_path_to_windows(char *path) >>>> strreplace(path, '/', '\\'); >>>> } >>>> -static char *casefold_sharename(struct unicode_map *um, const char >>>> *name) >>>> +char *ksmbd_casefold_sharename(struct unicode_map *um, const char >>>> *name) >>>> { >>>> char *cf_name; >>>> int cf_len; >>>> @@ -273,7 +273,7 @@ char *ksmbd_extract_sharename(struct unicode_map >>>> *um, const char *treename) >>>> name = (pos + 1); >>>> /* caller has to free the memory */ >>>> - return casefold_sharename(um, name); >>>> + return ksmbd_casefold_sharename(um, name); >>>> } >>>> /** >>>> diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h >>>> index cc72f4e6baf2..1facfcd21200 100644 >>>> --- a/fs/ksmbd/misc.h >>>> +++ b/fs/ksmbd/misc.h >>>> @@ -20,6 +20,7 @@ int get_nlink(struct kstat *st); >>>> void ksmbd_conv_path_to_unix(char *path); >>>> void ksmbd_strip_last_slash(char *path); >>>> void ksmbd_conv_path_to_windows(char *path); >>>> +char *ksmbd_casefold_sharename(struct unicode_map *um, const char >>>> *name); >>>> char *ksmbd_extract_sharename(struct unicode_map *um, const char >>>> *treename); >>>> char *convert_to_unix_name(struct ksmbd_share_config *share, const >>>> char *name); >>> >> >
diff --git a/fs/ksmbd/ksmbd_netlink.h b/fs/ksmbd/ksmbd_netlink.h index e0cbcfa98c7e..ff07c67f4565 100644 --- a/fs/ksmbd/ksmbd_netlink.h +++ b/fs/ksmbd/ksmbd_netlink.h @@ -163,7 +163,8 @@ struct ksmbd_share_config_response { __u16 force_directory_mode; __u16 force_uid; __u16 force_gid; - __u32 reserved[128]; /* Reserved room */ + __s8 share_name[KSMBD_REQ_MAX_SHARE_NAME]; + __u32 reserved[112]; /* Reserved room */ __u32 veto_list_sz; __s8 ____payload[]; }; diff --git a/fs/ksmbd/mgmt/share_config.c b/fs/ksmbd/mgmt/share_config.c index 5d039704c23c..c62e87b67247 100644 --- a/fs/ksmbd/mgmt/share_config.c +++ b/fs/ksmbd/mgmt/share_config.c @@ -16,6 +16,7 @@ #include "user_config.h" #include "user_session.h" #include "../transport_ipc.h" +#include "../misc.h" #define SHARE_HASH_BITS 3 static DEFINE_HASHTABLE(shares_table, SHARE_HASH_BITS); @@ -119,7 +120,8 @@ static int parse_veto_list(struct ksmbd_share_config *share, return 0; } -static struct ksmbd_share_config *share_config_request(const char *name) +static struct ksmbd_share_config *share_config_request(struct unicode_map *um, + const char *name) { struct ksmbd_share_config_response *resp; struct ksmbd_share_config *share = NULL; @@ -133,6 +135,17 @@ static struct ksmbd_share_config *share_config_request(const char *name) if (resp->flags == KSMBD_SHARE_FLAG_INVALID) goto out; + if (*resp->share_name) { + char *cf_resp_name; + bool equal; + + cf_resp_name = ksmbd_casefold_sharename(um, resp->share_name); + equal = cf_resp_name && !strcmp(cf_resp_name, name); + kfree(cf_resp_name); + if (!equal) + goto out; + } + share = kzalloc(sizeof(struct ksmbd_share_config), GFP_KERNEL); if (!share) goto out; @@ -190,7 +203,8 @@ static struct ksmbd_share_config *share_config_request(const char *name) return share; } -struct ksmbd_share_config *ksmbd_share_config_get(const char *name) +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map *um, + const char *name) { struct ksmbd_share_config *share; @@ -202,7 +216,7 @@ struct ksmbd_share_config *ksmbd_share_config_get(const char *name) if (share) return share; - return share_config_request(name); + return share_config_request(um, name); } bool ksmbd_share_veto_filename(struct ksmbd_share_config *share, diff --git a/fs/ksmbd/mgmt/share_config.h b/fs/ksmbd/mgmt/share_config.h index 7f7e89ecfe61..3fd338293942 100644 --- a/fs/ksmbd/mgmt/share_config.h +++ b/fs/ksmbd/mgmt/share_config.h @@ -9,6 +9,7 @@ #include <linux/workqueue.h> #include <linux/hashtable.h> #include <linux/path.h> +#include <linux/unicode.h> struct ksmbd_share_config { char *name; @@ -74,7 +75,8 @@ static inline void ksmbd_share_config_put(struct ksmbd_share_config *share) __ksmbd_share_config_put(share); } -struct ksmbd_share_config *ksmbd_share_config_get(const char *name); +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map *um, + const char *name); bool ksmbd_share_veto_filename(struct ksmbd_share_config *share, const char *filename); #endif /* __SHARE_CONFIG_MANAGEMENT_H__ */ diff --git a/fs/ksmbd/mgmt/tree_connect.c b/fs/ksmbd/mgmt/tree_connect.c index 867c0286b901..8ce17b3fb8da 100644 --- a/fs/ksmbd/mgmt/tree_connect.c +++ b/fs/ksmbd/mgmt/tree_connect.c @@ -26,7 +26,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess, struct sockaddr *peer_addr; int ret; - sc = ksmbd_share_config_get(share_name); + sc = ksmbd_share_config_get(conn->um, share_name); if (!sc) return status; @@ -61,7 +61,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess, struct ksmbd_share_config *new_sc; ksmbd_share_config_del(sc); - new_sc = ksmbd_share_config_get(share_name); + new_sc = ksmbd_share_config_get(conn->um, share_name); if (!new_sc) { pr_err("Failed to update stale share config\n"); status.ret = -ESTALE; diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c index 28459b1efaa8..9e8afaa686e3 100644 --- a/fs/ksmbd/misc.c +++ b/fs/ksmbd/misc.c @@ -227,7 +227,7 @@ void ksmbd_conv_path_to_windows(char *path) strreplace(path, '/', '\\'); } -static char *casefold_sharename(struct unicode_map *um, const char *name) +char *ksmbd_casefold_sharename(struct unicode_map *um, const char *name) { char *cf_name; int cf_len; @@ -273,7 +273,7 @@ char *ksmbd_extract_sharename(struct unicode_map *um, const char *treename) name = (pos + 1); /* caller has to free the memory */ - return casefold_sharename(um, name); + return ksmbd_casefold_sharename(um, name); } /** diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h index cc72f4e6baf2..1facfcd21200 100644 --- a/fs/ksmbd/misc.h +++ b/fs/ksmbd/misc.h @@ -20,6 +20,7 @@ int get_nlink(struct kstat *st); void ksmbd_conv_path_to_unix(char *path); void ksmbd_strip_last_slash(char *path); void ksmbd_conv_path_to_windows(char *path); +char *ksmbd_casefold_sharename(struct unicode_map *um, const char *name); char *ksmbd_extract_sharename(struct unicode_map *um, const char *treename); char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name);
Share config response may contain the share name without casefolding as it is known to the user space daemon. When it is present, casefold and compare it to the share name the share config request was made with. If they differ, we have a share config which is incompatible with the way share config caching is done. This is the case when CONFIG_UNICODE is not set, the share name contains non-ASCII characters, and those non- ASCII characters do not match those in the share name known to user space. In other words, when CONFIG_UNICODE is not set, UTF-8 share names now work but are only case-insensitive in the ASCII range. Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com> --- v3: - removed initial strcmp() check since it could only save a call to ksmbd_casefold_sharename() for matching ASCII-only share names v2: - no changes were made fs/ksmbd/ksmbd_netlink.h | 3 ++- fs/ksmbd/mgmt/share_config.c | 20 +++++++++++++++++--- fs/ksmbd/mgmt/share_config.h | 4 +++- fs/ksmbd/mgmt/tree_connect.c | 4 ++-- fs/ksmbd/misc.c | 4 ++-- fs/ksmbd/misc.h | 1 + 6 files changed, 27 insertions(+), 9 deletions(-)