Re: [PATCH v3 2/2] ksmbd: fix named-pipe reply loss with small output buffers

[Wang Zhaolong]

Hi Namjae,

> 
> if (rpc->pending)
>     rsp->hdr.Status = STATUS_BUFFER_OVERFLOW;
> Should we add this check ?
> 
>> +       goto done;
>>
>> +empty:
>> +       err = ksmbd_iov_pin_rsp(work, rsp,
>> +                               offsetof(struct smb2_read_rsp, Buffer));
>> +       if (err)
>> +               goto out_put;
>> +done:
>>         rsp->StructureSize = cpu_to_le16(17);
>>         rsp->DataOffset = 80;
>>         rsp->Reserved = 0;
>> @@ -9349,7 +9375,6 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
>>         rsp->hdr.Status = smb2_pipe_error_status(err,
>>                                                 STATUS_UNEXPECTED_IO_ERROR);
>>         smb2_set_err_rsp(work);
>> -       kvfree(rpc_resp);
>>         return err;
>>  }
>>
>> @@ -9947,7 +9972,7 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work)
>>         if (IS_ERR(rpc)) {
>>                 err = PTR_ERR(rpc);
>>                 rsp->hdr.Status = smb2_pipe_error_status(err,
>> -                                                       STATUS_INVALID_HANDLE);
>> +                                                        STATUS_INVALID_HANDLE);
>>                 smb2_set_err_rsp(work);
>>                 return err;
>>         }
>> @@ -11440,20 +11465,25 @@ static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
>>         struct ksmbd_rpc_command *rpc_resp;
>>         struct ksmbd_session_rpc *rpc;
>>         char *data_buf = (char *)req + le32_to_cpu(req->InputOffset);
>> -       unsigned int nbytes;
>> -       int err;
>> +       unsigned int nbytes = 0;
>> +       int ret;
>>
>>         rpc = ksmbd_session_rpc_get(work, id);
>>         if (IS_ERR(rpc)) {
>> -               err = PTR_ERR(rpc);
>> -               rsp->hdr.Status = smb2_pipe_error_status(err,
>> -                                                       STATUS_INVALID_PARAMETER);
>> +               ret = PTR_ERR(rpc);
>> +               rsp->hdr.Status = smb2_pipe_error_status(ret,
>> +                                                STATUS_INVALID_PARAMETER);
>>                 smb2_set_err_rsp(work);
>>                 return 0;
>>         }
>>
>> -       rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
>> -                                  le32_to_cpu(req->InputCount));
>> +       /* Write new input without reading past an already buffered reply. */
>> +       if (rpc->pending)
>> +               rpc_resp = ksmbd_rpc_write(work->sess, rpc, data_buf,
>> +                                          le32_to_cpu(req->InputCount));
>> +       else
>> +               rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
>> +                                          le32_to_cpu(req->InputCount));
> This WRITE leaves KSMBD_DCERPC_RETURN_READY set in mountd. After the
> cached reply is drained, the next TRANSCEIVE can return the previous
> reply without processing its own input.
> 
> Thanks.


Thanks for catching this.

I reproduced the sequence you described and confirmed the problem.

For the READ path, your suggested STATUS_BUFFER_OVERFLOW check looks
appropriate when a positive-length read leaves unread bytes from
the same reply.

For TRANSCEIVE, I think the correct fix needs some discussion.
One possibility is to return STATUS_PIPE_BUSY when cached reply
data remains, without issuing another IPC request or consuming the
cached data. The client would need to drain that reply through READ
before submitting another transaction. MS-FSCC §2.3.48[1] lists
PIPE_BUSY for a pipe containing unread data.

However, I would like to confirm that this is the intended behavior
for this SMB2 path before preparing v4. Simply omitting the WRITE
and returning cached data would still silently discard the input.

Would rejecting the transaction with STATUS_PIPE_BUSY be appropriate
here, or would you prefer a different approach?

[1] https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/f48cecf1-98d8-48dc-926f-2f3a48f07cf6

Thanks,
Wang Zhaolong