| Message ID | 20250306171959.853466-1-jeff.hugo@oss.qualcomm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | accel/qaic: Fix possible data corruption in BOs > 2G | expand |
On 3/6/25 09:19, Jeff Hugo wrote: > From: Jeffrey Hugo <quic_jhugo@quicinc.com> > > When slicing a BO, we need to iterate through the BO's sgt to find the > right pieces to construct the slice. Some of the data types chosen for > this process are incorrectly too small, and can overflow. This can > result in the incorrect slice construction, which can lead to data > corruption in workload execution. > > The device can only handle 32-bit sized transfers, and the scatterlist > struct only supports 32-bit buffer sizes, so our upper limit for an > individual transfer is an unsigned int. Using an int is incorrect due to > the reservation of the sign bit. Upgrade the length of a scatterlist > entry and the offsets into a scatterlist entry to unsigned int for a > correct representation. > > While each transfer may be limited to 32-bits, the overall BO may exceed > that size. For counting the total length of the BO, we need a type that > can represent the largest allocation possible on the system. That is the > definition of size_t, so use it. > > Fixes: ff13be830333 ("accel/qaic: Add datapath") > Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com> > Signed-off-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com> > --- > drivers/accel/qaic/qaic_data.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c > index c20eb63750f5..ffcdf5738d09 100644 > --- a/drivers/accel/qaic/qaic_data.c > +++ b/drivers/accel/qaic/qaic_data.c > @@ -172,9 +172,10 @@ static void free_slice(struct kref *kref) > static int clone_range_of_sgt_for_slice(struct qaic_device *qdev, struct sg_table **sgt_out, > struct sg_table *sgt_in, u64 size, u64 offset) > { > - int total_len, len, nents, offf = 0, offl = 0; > struct scatterlist *sg, *sgn, *sgf, *sgl; > + unsigned int len, nents, offf, offl; > struct sg_table *sgt; > + size_t total_len; > int ret, j; > > /* find out number of relevant nents needed for this mem */ > @@ -182,6 +183,8 @@ static int clone_range_of_sgt_for_slice(struct qaic_device *qdev, struct sg_tabl > sgf = NULL; > sgl = NULL; > nents = 0; > + offf = 0; > + offl = 0; Reviewed-by: Lizhi Hou <lizhi.hou@amd.com> > > size = size ? size : PAGE_SIZE; > for_each_sgtable_dma_sg(sgt_in, sg, j) {
On 3/6/25 12:19 PM, Jeff Hugo wrote: > From: Jeffrey Hugo <quic_jhugo@quicinc.com> > > When slicing a BO, we need to iterate through the BO's sgt to find the > right pieces to construct the slice. Some of the data types chosen for > this process are incorrectly too small, and can overflow. This can > result in the incorrect slice construction, which can lead to data > corruption in workload execution. > > The device can only handle 32-bit sized transfers, and the scatterlist > struct only supports 32-bit buffer sizes, so our upper limit for an > individual transfer is an unsigned int. Using an int is incorrect due to > the reservation of the sign bit. Upgrade the length of a scatterlist > entry and the offsets into a scatterlist entry to unsigned int for a > correct representation. > > While each transfer may be limited to 32-bits, the overall BO may exceed > that size. For counting the total length of the BO, we need a type that > can represent the largest allocation possible on the system. That is the > definition of size_t, so use it. > > Fixes: ff13be830333 ("accel/qaic: Add datapath") > Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com> > Signed-off-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com> Reviewed-by: Troy Hanson <quic_thanson@quicinc.com>
On 3/6/2025 5:19 PM, Jeff Hugo wrote: > From: Jeffrey Hugo <quic_jhugo@quicinc.com> > > When slicing a BO, we need to iterate through the BO's sgt to find the > right pieces to construct the slice. Some of the data types chosen for > this process are incorrectly too small, and can overflow. This can > result in the incorrect slice construction, which can lead to data > corruption in workload execution. > > The device can only handle 32-bit sized transfers, and the scatterlist > struct only supports 32-bit buffer sizes, so our upper limit for an > individual transfer is an unsigned int. Using an int is incorrect due to > the reservation of the sign bit. Upgrade the length of a scatterlist > entry and the offsets into a scatterlist entry to unsigned int for a > correct representation. > > While each transfer may be limited to 32-bits, the overall BO may exceed > that size. For counting the total length of the BO, we need a type that > can represent the largest allocation possible on the system. That is the > definition of size_t, so use it. > > Fixes: ff13be830333 ("accel/qaic: Add datapath") > Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com> > Signed-off-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com> Reviewed-by: Youssef Samir <quic_yabdulra@quicinc.com>
diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c index c20eb63750f5..ffcdf5738d09 100644 --- a/drivers/accel/qaic/qaic_data.c +++ b/drivers/accel/qaic/qaic_data.c @@ -172,9 +172,10 @@ static void free_slice(struct kref *kref) static int clone_range_of_sgt_for_slice(struct qaic_device *qdev, struct sg_table **sgt_out, struct sg_table *sgt_in, u64 size, u64 offset) { - int total_len, len, nents, offf = 0, offl = 0; struct scatterlist *sg, *sgn, *sgf, *sgl; + unsigned int len, nents, offf, offl; struct sg_table *sgt; + size_t total_len; int ret, j; /* find out number of relevant nents needed for this mem */ @@ -182,6 +183,8 @@ static int clone_range_of_sgt_for_slice(struct qaic_device *qdev, struct sg_tabl sgf = NULL; sgl = NULL; nents = 0; + offf = 0; + offl = 0; size = size ? size : PAGE_SIZE; for_each_sgtable_dma_sg(sgt_in, sg, j) {