From: Tom Talpey <tom@talpey.com>
To: "Namjae Jeon" <linkinjeon@kernel.org>,
"Atte Heikkilä" <atteh.mailbox@gmail.com>
Cc: linux-cifs@vger.kernel.org
Subject: Re: [PATCH v4] ksmbd: validate share name from share config response
Date: Mon, 3 Oct 2022 10:14:35 -0400 [thread overview]
Message-ID: <ed654e60-2a3b-22fe-2fcd-6f42e8905add@talpey.com> (raw)
In-Reply-To: <CAKYAXd__JiXd2YBnAmLwtBMuRkw4penZYrASsrxepm7UaZBBrw@mail.gmail.com>
On 10/2/2022 10:06 PM, Namjae Jeon wrote:
> 2022-10-03 6:21 GMT+09:00, Atte Heikkilä <atteh.mailbox@gmail.com>:
>> On Sun, 2 Oct 2022 14:34:43 -0400, Tom Talpey wrote:
>>> On 10/2/2022 11:40 AM, Atte Heikkilä wrote:
>>>> On Sun, 2 Oct 2022 05:46:28 +0300, Atte Heikkilä wrote:
>>>>> ...
>>>>> 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 */
>>>
>>> I notice you still have "112" here, did you reject my suggestion
>>> to code the size relative to KSMBD_REQ_MAX_SHARE_NAME?
>>>
>>
>> If size of `reserved' should be relative, then it would be:
>> 128 - DIV_ROUND_UP(KSMBD_REQ_MAX_SHARE_NAME, sizeof(__u32))
>>
>> Since ksmbd-tools already has the 112 (128-64/4), I thought to keep the
>> kernel side consistent with it for now.
>>
>>> Either way I think I made a flawed suggestion. The "reserved" field
>>> is __u32 but the KSMBD_REQ_MAX_SHARE_NAME is __s8. So, two things:
>>>
>>> - why is share_name an __s8? Wouldn't __u8 be more appropriate?
>>
>> As I understand it, the only reason for `__s8' over `__u8' here is that
>> `char' is most often a signed type.
>>
>>> - why is reserved a __u32? ISTM that __u8 would also be a better
>>> choice, and then the size would be "512 - KSMBD_REQ_MAX_SHARE_NAME".
>>>
>>>
>>
>> I don't know why `reserved' is `__u32'. I agree that it would be better
>> if it was `__u8'.
> We can make another patch for both ksmbd/ksmbd-tools later.
Two copies of this whole structure is a bug waiting to happen.
The best approach would be to place the definition in a single shared
header file. And it may need some sort of version stamp, if that huge
"reserved" block is to be used.
I believe the kernel include/uapi tree is an established place for this
type of user/kernel interface definition. A side benefit is that because
of the ABI implications, changes there are pretty well-vetted.
Tom.
>>
>>>>> __u32 veto_list_sz;
>>>>> __s8 ____payload[];
>>>>> };
>>>>> diff --git a/fs/ksmbd/mgmt/share_config.c
>>>>> b/fs/ksmbd/mgmt/share_config.c
>>>>> index 5d039704c23c..dfb4bb365891 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 = !IS_ERR(cf_resp_name) && !strcmp(cf_resp_name, name);
>>>>> + kfree(cf_resp_name);
>>>>
>>>> Well, kfree() is *not* a no-op for ERR_PTR() like it is for NULL so
>>>> this patch is not good either. At least I'm running out of ways to get
>>>> this wrong.
>>>
>>> :)
>>>
>>> Tom.
>>>
>>>
>>>>
>>>>> + 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);
>>>>>
>>>>> --
>>>>> 2.37.3
>>>>>
>>>>>
>>>>
>>>
>>
>
prev parent reply other threads:[~2022-10-03 14:14 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-02 2:46 [PATCH v4] ksmbd: validate share name from share config response Atte Heikkilä
2022-10-02 15:40 ` Atte Heikkilä
2022-10-02 18:34 ` Tom Talpey
2022-10-02 21:21 ` Atte Heikkilä
2022-10-03 2:06 ` Namjae Jeon
2022-10-03 14:14 ` Tom Talpey [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ed654e60-2a3b-22fe-2fcd-6f42e8905add@talpey.com \
--to=tom@talpey.com \
--cc=atteh.mailbox@gmail.com \
--cc=linkinjeon@kernel.org \
--cc=linux-cifs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox