| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <F9BB72E1C6B6AFFD+37b5bda8-2488-439a-8203-e0c376fae679@chenxiaosong.com>
Date: Thu, 22 Aug 2024 08:59:42 +0800
From: ChenXiaoSong <chenxiaosong@...nxiaosong.com>
To: Namjae Jeon <linkinjeon@...nel.org>
Cc: sfrench@...ba.org, senozhatsky@...omium.org, tom@...pey.com,
linux-cifs@...r.kernel.org, linux-kernel@...r.kernel.org, pc@...guebit.com,
ronniesahlberg@...il.com, sprasad@...rosoft.com, bharathsm@...rosoft.com,
chenxiaosong@...inos.cn, liuzhengyuan@...inos.cn, huhai@...inos.cn,
liuyun01@...inos.cn
Subject: Re: [PATCH 2/8] smb/server: fix potential null-ptr-deref of
lease_ctx_info in smb2_open()
Thanks for your reply. I will send v2 patchset soon based on your
suggestions.
在 2024/8/22 08:41, Namjae Jeon 写道:
> On Tue, Aug 20, 2024 at 11:35 PM <chenxiaosong@...nxiaosong.com> wrote:
>>
>> From: ChenXiaoSong <chenxiaosong@...inos.cn>
>>
>> null-ptr-deref will occur when (req_op_level == SMB2_OPLOCK_LEVEL_LEASE)
>> and parse_lease_state() return NULL.
>>
>> Fix this by returning error pointer on parse_lease_state() and checking
>> error.
>>
>> Signed-off-by: ChenXiaoSong <chenxiaosong@...nxiaosong.com>
> We intended for it to return null. We shouldn't handle the error even
> if it fails.
> All places check if lc is null except the one.
> We can fix it like the following one. please send this patch if you are okay.
>
> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
> index b6c5a8ea3887..884e21992c92 100644
> --- a/fs/smb/server/smb2pdu.c
> +++ b/fs/smb/server/smb2pdu.c
> @@ -3404,7 +3404,7 @@ int smb2_open(struct ksmbd_work *work)
> goto err_out1;
> }
> } else {
> - if (req_op_level == SMB2_OPLOCK_LEVEL_LEASE) {
> + if (req_op_level == SMB2_OPLOCK_LEVEL_LEASE && lc) {
> if (S_ISDIR(file_inode(filp)->i_mode)) {
> lc->req_state &= ~SMB2_LEASE_WRITE_CACHING_LE;
> lc->is_dir = true;
>
>
>> ---
>> fs/smb/server/oplock.c | 11 +++++++----
>> fs/smb/server/smb2pdu.c | 17 ++++++++++++-----
>> 2 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/smb/server/oplock.c b/fs/smb/server/oplock.c
>> index a8f52c4ebbda..e8591686a037 100644
>> --- a/fs/smb/server/oplock.c
>> +++ b/fs/smb/server/oplock.c
>> @@ -1510,7 +1510,8 @@ void create_lease_buf(u8 *rbuf, struct lease *lease)
>> * parse_lease_state() - parse lease context containted in file open request
>> * @open_req: buffer containing smb2 file open(create) request
>> *
>> - * Return: oplock state, -ENOENT if create lease context not found
>> + * Return: allocated lease context object on success, otherwise error pointer.
>> + * -ENOENT pointer if create lease context not found.
>> */
>> struct lease_ctx_info *parse_lease_state(void *open_req)
>> {
>> @@ -1519,12 +1520,14 @@ struct lease_ctx_info *parse_lease_state(void *open_req)
>> struct lease_ctx_info *lreq;
>>
>> cc = smb2_find_context_vals(req, SMB2_CREATE_REQUEST_LEASE, 4);
>> - if (IS_ERR_OR_NULL(cc))
>> - return NULL;
>> + if (!cc)
>> + return ERR_PTR(-ENOENT);
>> + if (IS_ERR(cc))
>> + return ERR_CAST(cc);
>>
>> lreq = kzalloc(sizeof(struct lease_ctx_info), GFP_KERNEL);
>> if (!lreq)
>> - return NULL;
>> + return ERR_PTR(-ENOMEM);
>>
>> if (sizeof(struct lease_context_v2) == le32_to_cpu(cc->DataLength)) {
>> struct create_lease_v2 *lc = (struct create_lease_v2 *)cc;
>> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
>> index d8a827e0dced..119c1ba5f255 100644
>> --- a/fs/smb/server/smb2pdu.c
>> +++ b/fs/smb/server/smb2pdu.c
>> @@ -2767,8 +2767,9 @@ static int parse_durable_handle_context(struct ksmbd_work *work,
>> }
>> }
>>
>> - if (((lc && (lc->req_state & SMB2_LEASE_HANDLE_CACHING_LE)) ||
>> - req_op_level == SMB2_OPLOCK_LEVEL_BATCH)) {
>> + if ((!IS_ERR_OR_NULL(lc) > 0 &&
>> + (lc->req_state & SMB2_LEASE_HANDLE_CACHING_LE)) ||
>> + req_op_level == SMB2_OPLOCK_LEVEL_BATCH) {
>> dh_info->CreateGuid =
>> durable_v2_blob->CreateGuid;
>> dh_info->persistent =
>> @@ -2788,8 +2789,9 @@ static int parse_durable_handle_context(struct ksmbd_work *work,
>> goto out;
>> }
>>
>> - if (((lc && (lc->req_state & SMB2_LEASE_HANDLE_CACHING_LE)) ||
>> - req_op_level == SMB2_OPLOCK_LEVEL_BATCH)) {
>> + if ((!IS_ERR_OR_NULL(lc) &&
>> + (lc->req_state & SMB2_LEASE_HANDLE_CACHING_LE)) ||
>> + req_op_level == SMB2_OPLOCK_LEVEL_BATCH) {
>> ksmbd_debug(SMB, "Request for durable open\n");
>> dh_info->type = dh_idx;
>> }
>> @@ -2935,8 +2937,13 @@ int smb2_open(struct ksmbd_work *work)
>> ksmbd_put_durable_fd(fp);
>> goto reconnected_fp;
>> }
>> - } else if (req_op_level == SMB2_OPLOCK_LEVEL_LEASE)
>> + } else if (req_op_level == SMB2_OPLOCK_LEVEL_LEASE) {
>> lc = parse_lease_state(req);
>> + if (IS_ERR(lc)) {
>> + rc = PTR_ERR(lc);
>> + goto err_out2;
>> + }
>> + }
>>
>> if (le32_to_cpu(req->ImpersonationLevel) > le32_to_cpu(IL_DELEGATE)) {
>> pr_err("Invalid impersonationlevel : 0x%x\n",
>> --
>> 2.34.1
>>
>
Powered by blists - more mailing lists