[PATCH 2/4] target/user: Return an error if cmd data size is too large

[Tim Gardner]

On 12/07/2016 09:01 AM, Seth Forshee wrote:
> On Wed, Dec 07, 2016 at 08:35:05AM -0700, Tim Gardner wrote:
>> On 12/07/2016 07:57 AM, Seth Forshee wrote:
>>> On Tue, Dec 06, 2016 at 12:40:53PM -0700, Tim Gardner wrote:
>>>> From: Andy Grover <agrover at redhat.com>
>>>>
>>>> BugLink: http://bugs.launchpad.net/bugs/1646204
>>>>
>>>> Userspace should be implementing VPD B0 (Block Limits) to inform the
>>>> initiator of max data size, but just in case we do get a too-large request,
>>>> do what the spec says and return INVALID_CDB_FIELD.
>>>>
>>>> Make sure to unlock udev->cmdr_lock before returning.
>>>>
>>>> Signed-off-by: Andy Grover <agrover at redhat.com>
>>>> Reviewed-by: Christoph Hellwig <hch at lst.de>
>>>> Reviewed-by: Mike Christie <mchristi at redhat.com>
>>>> Signed-off-by: Nicholas Bellinger <nab at linux-iscsi.org>
>>>> (back ported from commit 554617b2bbe25c3fb3c80c28fe7a465884bb40b1)
>>>> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
>>>>
>>>> Conflicts:
>>>> 	drivers/target/target_core_user.c
>>>> ---
>>>>  drivers/target/target_core_user.c | 17 +++++++++++++----
>>>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
>>>> index 28debdf..c252528 100644
>>>> --- a/drivers/target/target_core_user.c
>>>> +++ b/drivers/target/target_core_user.c
>>>> @@ -393,6 +393,7 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
>>>>  	uint32_t cmd_head;
>>>>  	uint64_t cdb_off;
>>>>  	bool copy_to_data_area;
>>>> +	size_t data_length;
>>>>  
>>>>  	if (test_bit(TCMU_DEV_BIT_BROKEN, &udev->flags))
>>>>  		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>>>> @@ -417,11 +418,19 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
>>>>  
>>>>  	mb = udev->mb_addr;
>>>>  	cmd_head = mb->cmd_head % udev->cmdr_size; /* UAM */
>>>> -	if ((command_size > (udev->cmdr_size / 2))
>>>> -	    || tcmu_cmd->data_length > (udev->data_size - 1))
>>>> -		pr_warn("TCMU: Request of size %zu/%zu may be too big for %u/%zu "
>>>> -			"cmd/data ring buffers\n", command_size, tcmu_cmd->data_length,
>>>> +	data_length = se_cmd->data_length;
>>>> +	if (se_cmd->se_cmd_flags & SCF_BIDI) {
>>>> +		BUG_ON(!(se_cmd->t_bidi_data_sg && se_cmd->t_bidi_data_nents));
>>>> +		data_length += se_cmd->t_bidi_data_sg->length;
>>>> +	}
>>>
>>> This really backports more changes than are necessary. This part
>>> duplicates code in tcmu_alloc_cmd(), which was moved here by
>>> 26418649eead "target/user: Introduce data_bitmap, replace
>>> data_length/data_head/data_tail" because the data_length member of
>>> tcmu_cmd was removed.
>>>
>>>> +	if ((command_size > (udev->cmdr_size / 2)) ||
>>>> +	    data_length > udev->data_size) {
>>>> +		pr_warn("TCMU: Request of size %zu/%zu is too big for %u/%zu "
>>>> +			"cmd/data ring buffers\n", command_size, data_length,
>>>>  			udev->cmdr_size, udev->data_size);
>>>> +		spin_unlock_irq(&udev->cmdr_lock);
>>>> +		return TCM_INVALID_CDB_FIELD;
>>>> +	}
>>>
>>> And so this part should be able to continue using tcmu_cmd->data_length
>>> as before. All that's really needed is to change this chunk to look like
>>> this:
>>>
>>>         if ((command_size > (udev->cmdr_size / 2)) ||
>>>             tcmu_cmd->data_length > (udev->data_size - 1)) {
>>>                 pr_warn("TCMU: Request of size %zu/%zu may be too big for %u/%zu "
>>>                         "cmd/data ring buffers\n", command_size, tcmu_cmd->data_length,
>>>                         udev->cmdr_size, udev->data_size);
>>>                 spin_unlock_irq(&udev->cmdr_lock);
>>>                 return TCM_INVALID_CDB_FIELD;
>>>         }
>>>
>>> (I'm not sure about the (udev->data_size - 1) -> udev->data_size part,
>>> was also changed in 26418649eead without explanation so maybe an
>>> off-by-one error.)
>>>
>>> I don't think your backport actually breaks anything though, it just
>>> backports more than is necessary.
>>>
>>
>> I tried to adhere to the spirit of the upstream commit so that future
>> patches against this code won't have giant conflicts. As for
>> data_length, the comparison of (data_length > udev->data_size) ought to
>> be correct.
> 
> Okay, if that's what you want then I won't object. Like I said I don't
> think it will introduce any regressions.
> 
>> I don't see how that could be an off by one error.
> 
> What I meant was that (udev->data_size - 1) was probably an off by one
> error which was fixed in 26418649eead. Now that I think about it again
> though maybe it wasn't an error. Based on the code I think it's
> intentionally leaving a free byte in the ring buffer, for example:
> 
> static inline size_t spc_free(size_t head, size_t tail, size_t size)
> {
>         /* Keep 1 byte unused or we can't tell full from empty */
>         return (size - spc_used(head, tail, size) - 1);
> }
> 
> Since xenial still uses the ring buffer implementation I think you need
> to leave it as (udev->data_size - 1).
> 
> Seth
> 

OK, I'll redo this pile and send a V2.
-- 
Tim Gardner tim.gardner at canonical.com