Re: [PATCH 1/1] scsi: target: core: Add 8Fh VPD page

[David Disseldorp <ddiss@suse.de>]

On Mon, 16 Aug 2021 18:16:45 +0000, Sergey Samoylenko wrote:

> Hi David,
> 
> > Hi Sergey,
> >
> > On Thu, 29 Jul 2021 23:19:43 +0300, Sergey Samoylenko wrote:
> >  
> >> The 8Fh VPD page announces the capabilities supported by
> >> the TCM XCOPY manager. It helps to expand the coverage of
> >> the third-party copy manager with SCSI testing utilities.  
> >
> > Please list which initiators use this VPD page, if you know of any.  
> I know that the ESXi 7.0 requests the 8Fh VPD page.

Thanks. Please put this in the commit message.

> ESXi is one of
> a few initiators who is using the XCOPY commands (vmkfstools tool).
> 
> > Also, is there any test coverage for this? I don't see anything in
> > libiscsi...  
> After activating XCOPY in a target we got an error from
> the SCSI.ReceiveCopyResults.CopyStatus test in the libiscsi.
> Discussing with Bart, we decided to implement the 8Fh VPD page
> for announcing TCM XCOPY features.
> It is here: https://github.com/sahlberg/libiscsi/pull/353
> 
> The libiscsi has an initial version for parsing 8Fh VPD. This is used
> in the SCSI.ReceiveCopyResults.CopyStatus test. It is a good idea
> to add test coverage for 8Fh VPD in libiscsi. I should do this.
> 
> >  
> >> Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
> >> Reviewed-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> >> Reviewed-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
> >> Signed-off-by: Sergey Samoylenko <s.samoylenko@yadro.com>
> >> ---
> >>  drivers/target/target_core_spc.c | 230 ++++++++++++++++++++++++++++++-
> >>  1 file changed, 226 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> >> index 22703a0dbd07..169341712b10 100644
> >> --- a/drivers/target/target_core_spc.c
> >> +++ b/drivers/target/target_core_spc.c  
> > ...  
> >> +/* Third-party Copy VPD page */
> >> +static sense_reason_t
> >> +spc_emulate_evpd_8f(struct se_cmd *cmd, unsigned char *buf)
> >> +{
> >> +	struct se_device *dev = cmd->se_dev;
> >> +	int off;
> >> +	u16 page_len;
> >> +
> >> +	if (!dev->dev_attrib.emulate_3pc)
> >> +		return TCM_INVALID_CDB_FIELD;
> >> +
> >> +	/*
> >> +	 * Since the Third-party copy manager in TCM is quite simple
> >> +	 * and supports only two commands, the function sets
> >> +	 * many descriptor parameters as constants.
> >> +	 *
> >> +	 * As the Copy manager supports the EXTENDED COPY(LID1) command,
> >> +	 * the Third-party Copy VPD page should include five mandatory
> >> +	 * Third-party copy descriptors. Its are:
> >> +	 *   0001h - Supported Commands
> >> +	 *   0004h - Parameter Data
> >> +	 *   0008h - Supported Descriptors
> >> +	 *   000Ch - Supported CSCD Descriptor IDs
> >> +	 *   8001h - General Copy Operations
> >> +	 *
> >> +	 * See spc4 section 7.8.17
> >> +	 */
> >> +
> >> +	off = 4;
> >> +
> >> +	/* fill descriptors */
> >> +	off += spc_evpd_8f_encode_supp_cmds(&buf[off]);
> >> +	off += spc_evpd_8f_encode_param_data(&buf[off]);
> >> +	off += spc_evpd_8f_encode_supp_descrs(&buf[off]);
> >> +	off += spc_evpd_8f_encode_supp_cscd_descr_id(&buf[off]);
> >> +	off += spc_evpd_8f_encode_general_copy_ops(&buf[off]);  
> >
> > This looks risky in terms of buf overrun. I think it'd be good to pass
> > a @remaining or @buf_end param to these helper functions.  
> 
> I thought about it, but spc_emulate_evpd_XX functions have a prototype:
> 	sense_reason_t	(*emulate)(struct se_cmd *, unsigned char *);
> and they don't know anything about buffer length. I can use the
> "SE_INQUIRY_BUF" definition, but I don't like this solution.
> 	
> We can change the prototype of spc_emulate_evpd_XX functions, like:
> 	static struct {
> 		uint8_t page;
> 		sense_reason_t	(*emulate)(struct se_cmd *, unsigned char *buf, size_t len);
> 	} evpd_handlers[] = {
> 		...
> 	};
> and return the TCM_OUT_OF_RESOURCES if we try to overrun buffer
> in spc_emulate_evpd_XX. But this will require changing all "emulate_evpd" functions.
> 
> David, what do you think of this?

Ideally inquiry and mode sense handlers, not to mention the configfs
callbacks, would all carry explicit bounds checks. As a start I'd be
fine with buflen=SE_INQUIRY_BUF at the top of spc_emulate_evpd_8f(), but
any further steps towards doing it properly would be helpful IMO.

Cheers, David