| Message ID | 20230119094934.86067-1-rrichter@amd.com |
|---|---|
| State | Accepted |
| Commit | 623c0751336e4035ab0047f2c152a02bd26b612b |
| Headers | show |
| Series | [v4] cxl/mbox: Fix Payload Length check for Get Log command | expand |
On Thu, 19 Jan 2023 10:49:34 +0100 Robert Richter <rrichter@amd.com> wrote: > Commit 2aeaf663b85e introduced strict checking for variable length > payload size validation. The payload length of received data must > match the size of the requested data by the caller except for the case > where the min_out value is set. > > The Get Log command does not have a header with a length field set. > The Log size is determined by the Get Supported Logs command (CXL 3.0, > 8.2.9.5.1). However, the actual size can be smaller and the number of > valid bytes in the payload output must be determined reading the > Payload Length field (CXL 3.0, Table 8-36, Note 2). > > Two issues arise: The command can successfully complete with a payload > length of zero. And, the valid payload length must then also be > consumed by the caller. > > Change cxl_xfer_log() to pass the number of payload bytes back to the > caller to determine the number of log entries. Implement the payload > handling as a special case where mbox_cmd->size_out is consulted when > cxl_internal_send_cmd() returns -EIO. A WARN_ONCE() is added to check > that -EIO is only returned in case of an unexpected output size. > > Logs can be bigger than the maximum payload length and multiple Get > Log commands can be issued. If the received payload size is smaller > than the maximum payload size we can assume all valid bytes have been > fetched. Stop sending further Get Log commands then. > > On that occasion, change debug messages to also report the opcodes of > supported commands. > > The variable payload commands GET_LSA and SET_LSA are not affected by > this strict check: SET_LSA cannot be broken because SET_LSA does not > return an output payload, and GET_LSA never expects short reads. > > Fixes: 2aeaf663b85e ("cxl/mbox: Add variable output size validation for internal commands") > Signed-off-by: Robert Richter <rrichter@amd.com> LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > v4: > * Fixed a build issue. > v3: > * Added comment to the WARN_ONCE(). (Jonathan) > * Passing a size pointer to report the payload size back. (Jonathan) > * Moved logging of supported opcodes out of this patch for a separate > change. (Jonathan) > > drivers/cxl/core/mbox.c | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index b03fba212799..a48ade466d6a 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -170,6 +170,12 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds, > out_size = mbox_cmd->size_out; > min_out = mbox_cmd->min_out; > rc = cxlds->mbox_send(cxlds, mbox_cmd); > + /* > + * EIO is reserved for a payload size mismatch and mbox_send() > + * may not return this error. > + */ > + if (WARN_ONCE(rc == -EIO, "Bad return code: -EIO")) > + return -ENXIO; > if (rc) > return rc; > > @@ -550,9 +556,9 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) > return 0; > } > > -static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 *out) > +static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 *size, u8 *out) > { > - u32 remaining = size; > + u32 remaining = *size; > u32 offset = 0; > > while (remaining) { > @@ -576,6 +582,17 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 > }; > > rc = cxl_internal_send_cmd(cxlds, &mbox_cmd); > + > + /* > + * The output payload length that indicates the number > + * of valid bytes can be smaller than the Log buffer > + * size. > + */ > + if (rc == -EIO && mbox_cmd.size_out < xfer_size) { > + offset += mbox_cmd.size_out; > + break; > + } > + > if (rc < 0) > return rc; > > @@ -584,6 +601,8 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 > offset += xfer_size; > } > > + *size = offset; > + > return 0; > } > > @@ -694,7 +713,7 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds) > goto out; > } > > - rc = cxl_xfer_log(cxlds, &uuid, size, log); > + rc = cxl_xfer_log(cxlds, &uuid, &size, log); > if (rc) { > kvfree(log); > goto out; > > base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
On 1/19/23 2:49 AM, Robert Richter wrote: > Commit 2aeaf663b85e introduced strict checking for variable length > payload size validation. The payload length of received data must > match the size of the requested data by the caller except for the case > where the min_out value is set. > > The Get Log command does not have a header with a length field set. > The Log size is determined by the Get Supported Logs command (CXL 3.0, > 8.2.9.5.1). However, the actual size can be smaller and the number of > valid bytes in the payload output must be determined reading the > Payload Length field (CXL 3.0, Table 8-36, Note 2). > > Two issues arise: The command can successfully complete with a payload > length of zero. And, the valid payload length must then also be > consumed by the caller. > > Change cxl_xfer_log() to pass the number of payload bytes back to the > caller to determine the number of log entries. Implement the payload > handling as a special case where mbox_cmd->size_out is consulted when > cxl_internal_send_cmd() returns -EIO. A WARN_ONCE() is added to check > that -EIO is only returned in case of an unexpected output size. > > Logs can be bigger than the maximum payload length and multiple Get > Log commands can be issued. If the received payload size is smaller > than the maximum payload size we can assume all valid bytes have been > fetched. Stop sending further Get Log commands then. > > On that occasion, change debug messages to also report the opcodes of > supported commands. > > The variable payload commands GET_LSA and SET_LSA are not affected by > this strict check: SET_LSA cannot be broken because SET_LSA does not > return an output payload, and GET_LSA never expects short reads. > > Fixes: 2aeaf663b85e ("cxl/mbox: Add variable output size validation for internal commands") > Signed-off-by: Robert Richter <rrichter@amd.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > v4: > * Fixed a build issue. > v3: > * Added comment to the WARN_ONCE(). (Jonathan) > * Passing a size pointer to report the payload size back. (Jonathan) > * Moved logging of supported opcodes out of this patch for a separate > change. (Jonathan) > > drivers/cxl/core/mbox.c | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index b03fba212799..a48ade466d6a 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -170,6 +170,12 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds, > out_size = mbox_cmd->size_out; > min_out = mbox_cmd->min_out; > rc = cxlds->mbox_send(cxlds, mbox_cmd); > + /* > + * EIO is reserved for a payload size mismatch and mbox_send() > + * may not return this error. > + */ > + if (WARN_ONCE(rc == -EIO, "Bad return code: -EIO")) > + return -ENXIO; > if (rc) > return rc; > > @@ -550,9 +556,9 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) > return 0; > } > > -static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 *out) > +static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 *size, u8 *out) > { > - u32 remaining = size; > + u32 remaining = *size; > u32 offset = 0; > > while (remaining) { > @@ -576,6 +582,17 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 > }; > > rc = cxl_internal_send_cmd(cxlds, &mbox_cmd); > + > + /* > + * The output payload length that indicates the number > + * of valid bytes can be smaller than the Log buffer > + * size. > + */ > + if (rc == -EIO && mbox_cmd.size_out < xfer_size) { > + offset += mbox_cmd.size_out; > + break; > + } > + > if (rc < 0) > return rc; > > @@ -584,6 +601,8 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 > offset += xfer_size; > } > > + *size = offset; > + > return 0; > } > > @@ -694,7 +713,7 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds) > goto out; > } > > - rc = cxl_xfer_log(cxlds, &uuid, size, log); > + rc = cxl_xfer_log(cxlds, &uuid, &size, log); > if (rc) { > kvfree(log); > goto out; > > base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
Robert Richter wrote: > Commit 2aeaf663b85e introduced strict checking for variable length > payload size validation. The payload length of received data must > match the size of the requested data by the caller except for the case > where the min_out value is set. > > The Get Log command does not have a header with a length field set. > The Log size is determined by the Get Supported Logs command (CXL 3.0, > 8.2.9.5.1). However, the actual size can be smaller and the number of > valid bytes in the payload output must be determined reading the > Payload Length field (CXL 3.0, Table 8-36, Note 2). > > Two issues arise: The command can successfully complete with a payload > length of zero. And, the valid payload length must then also be > consumed by the caller. > > Change cxl_xfer_log() to pass the number of payload bytes back to the > caller to determine the number of log entries. Implement the payload > handling as a special case where mbox_cmd->size_out is consulted when > cxl_internal_send_cmd() returns -EIO. A WARN_ONCE() is added to check > that -EIO is only returned in case of an unexpected output size. > > Logs can be bigger than the maximum payload length and multiple Get > Log commands can be issued. If the received payload size is smaller > than the maximum payload size we can assume all valid bytes have been > fetched. Stop sending further Get Log commands then. > > On that occasion, change debug messages to also report the opcodes of > supported commands. > > The variable payload commands GET_LSA and SET_LSA are not affected by > this strict check: SET_LSA cannot be broken because SET_LSA does not > return an output payload, and GET_LSA never expects short reads. > > Fixes: 2aeaf663b85e ("cxl/mbox: Add variable output size validation for internal commands") > Signed-off-by: Robert Richter <rrichter@amd.com> > --- > v4: > * Fixed a build issue. > v3: > * Added comment to the WARN_ONCE(). (Jonathan) > * Passing a size pointer to report the payload size back. (Jonathan) > * Moved logging of supported opcodes out of this patch for a separate > change. (Jonathan) > > drivers/cxl/core/mbox.c | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) Looks good Robert. Do you think this is v6.2-rc material or can it wait for v6.3? It sounds like you have a real device that needs, so I am inclined to add it to cxl/fixes for this cycle.
On 27.01.23 11:48:59, Dan Williams wrote: > Robert Richter wrote: > > Commit 2aeaf663b85e introduced strict checking for variable length > > payload size validation. The payload length of received data must > > match the size of the requested data by the caller except for the case > > where the min_out value is set. > > > > The Get Log command does not have a header with a length field set. > > The Log size is determined by the Get Supported Logs command (CXL 3.0, > > 8.2.9.5.1). However, the actual size can be smaller and the number of > > valid bytes in the payload output must be determined reading the > > Payload Length field (CXL 3.0, Table 8-36, Note 2). > > > > Two issues arise: The command can successfully complete with a payload > > length of zero. And, the valid payload length must then also be > > consumed by the caller. > > > > Change cxl_xfer_log() to pass the number of payload bytes back to the > > caller to determine the number of log entries. Implement the payload > > handling as a special case where mbox_cmd->size_out is consulted when > > cxl_internal_send_cmd() returns -EIO. A WARN_ONCE() is added to check > > that -EIO is only returned in case of an unexpected output size. > > > > Logs can be bigger than the maximum payload length and multiple Get > > Log commands can be issued. If the received payload size is smaller > > than the maximum payload size we can assume all valid bytes have been > > fetched. Stop sending further Get Log commands then. > > > > On that occasion, change debug messages to also report the opcodes of > > supported commands. > > > > The variable payload commands GET_LSA and SET_LSA are not affected by > > this strict check: SET_LSA cannot be broken because SET_LSA does not > > return an output payload, and GET_LSA never expects short reads. > > > > Fixes: 2aeaf663b85e ("cxl/mbox: Add variable output size validation for internal commands") > > Signed-off-by: Robert Richter <rrichter@amd.com> > > --- > > v4: > > * Fixed a build issue. > > v3: > > * Added comment to the WARN_ONCE(). (Jonathan) > > * Passing a size pointer to report the payload size back. (Jonathan) > > * Moved logging of supported opcodes out of this patch for a separate > > change. (Jonathan) > > > > drivers/cxl/core/mbox.c | 25 ++++++++++++++++++++++--- > > 1 file changed, 22 insertions(+), 3 deletions(-) > > Looks good Robert. Do you think this is v6.2-rc material or can it wait > for v6.3? It sounds like you have a real device that needs, so I am > inclined to add it to cxl/fixes for this cycle. 6.3 is ok to me. Thanks, -Robert
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index b03fba212799..a48ade466d6a 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -170,6 +170,12 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds, out_size = mbox_cmd->size_out; min_out = mbox_cmd->min_out; rc = cxlds->mbox_send(cxlds, mbox_cmd); + /* + * EIO is reserved for a payload size mismatch and mbox_send() + * may not return this error. + */ + if (WARN_ONCE(rc == -EIO, "Bad return code: -EIO")) + return -ENXIO; if (rc) return rc; @@ -550,9 +556,9 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) return 0; } -static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 *out) +static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 *size, u8 *out) { - u32 remaining = size; + u32 remaining = *size; u32 offset = 0; while (remaining) { @@ -576,6 +582,17 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 }; rc = cxl_internal_send_cmd(cxlds, &mbox_cmd); + + /* + * The output payload length that indicates the number + * of valid bytes can be smaller than the Log buffer + * size. + */ + if (rc == -EIO && mbox_cmd.size_out < xfer_size) { + offset += mbox_cmd.size_out; + break; + } + if (rc < 0) return rc; @@ -584,6 +601,8 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 offset += xfer_size; } + *size = offset; + return 0; } @@ -694,7 +713,7 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds) goto out; } - rc = cxl_xfer_log(cxlds, &uuid, size, log); + rc = cxl_xfer_log(cxlds, &uuid, &size, log); if (rc) { kvfree(log); goto out;
Commit 2aeaf663b85e introduced strict checking for variable length payload size validation. The payload length of received data must match the size of the requested data by the caller except for the case where the min_out value is set. The Get Log command does not have a header with a length field set. The Log size is determined by the Get Supported Logs command (CXL 3.0, 8.2.9.5.1). However, the actual size can be smaller and the number of valid bytes in the payload output must be determined reading the Payload Length field (CXL 3.0, Table 8-36, Note 2). Two issues arise: The command can successfully complete with a payload length of zero. And, the valid payload length must then also be consumed by the caller. Change cxl_xfer_log() to pass the number of payload bytes back to the caller to determine the number of log entries. Implement the payload handling as a special case where mbox_cmd->size_out is consulted when cxl_internal_send_cmd() returns -EIO. A WARN_ONCE() is added to check that -EIO is only returned in case of an unexpected output size. Logs can be bigger than the maximum payload length and multiple Get Log commands can be issued. If the received payload size is smaller than the maximum payload size we can assume all valid bytes have been fetched. Stop sending further Get Log commands then. On that occasion, change debug messages to also report the opcodes of supported commands. The variable payload commands GET_LSA and SET_LSA are not affected by this strict check: SET_LSA cannot be broken because SET_LSA does not return an output payload, and GET_LSA never expects short reads. Fixes: 2aeaf663b85e ("cxl/mbox: Add variable output size validation for internal commands") Signed-off-by: Robert Richter <rrichter@amd.com> --- v4: * Fixed a build issue. v3: * Added comment to the WARN_ONCE(). (Jonathan) * Passing a size pointer to report the payload size back. (Jonathan) * Moved logging of supported opcodes out of this patch for a separate change. (Jonathan) drivers/cxl/core/mbox.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2