The split_sg_phys function was incorrectly setting the offsets of all
scatterlist entries (except the first) to 0. Only the first scatterlist
entry's offset and length needs to be modified to account for the skip.
Setting the rest entries' offsets to 0 could lead to incorrect data
access.

This patch removes the offending code, ensuring that the page offsets
in the input scatterlist are preserved in the output scatterlist.

Fixes: f8bcbe62acd0 ("lib: scatterlist: add sg splitting function")
Signed-off-by: T Pratham <t-pratham@ti.com>
---
 lib/sg_split.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/sg_split.c b/lib/sg_split.c
index 60a0babebf2e..0f89aab5c671 100644
--- a/lib/sg_split.c
+++ b/lib/sg_split.c
@@ -88,8 +88,6 @@ static void sg_split_phys(struct sg_splitter *splitters, const int nb_splits)
 			if (!j) {
 				out_sg->offset += split->skip_sg0;
 				out_sg->length -= split->skip_sg0;
-			} else {
-				out_sg->offset = 0;
 			}
 			sg_dma_address(out_sg) = 0;
 			sg_dma_len(out_sg) = 0;
-- 
2.34.1

On Wed, 19 Mar 2025 16:44:38 +0530 T Pratham <t-pratham@ti.com> wrote:

> The split_sg_phys function was incorrectly setting the offsets of all
> scatterlist entries (except the first) to 0. Only the first scatterlist
> entry's offset and length needs to be modified to account for the skip.
> Setting the rest entries' offsets to 0 could lead to incorrect data
> access.
> 
> This patch removes the offending code, ensuring that the page offsets
> in the input scatterlist are preserved in the output scatterlist.

Is this merely from code inspection, or is this issues known to have
observable runtime effects?

If the latter, please provide a complete description.

>
> ...
>
> --- a/lib/sg_split.c
> +++ b/lib/sg_split.c
> @@ -88,8 +88,6 @@ static void sg_split_phys(struct sg_splitter *splitters, const int nb_splits)
>  			if (!j) {
>  				out_sg->offset += split->skip_sg0;
>  				out_sg->length -= split->skip_sg0;
> -			} else {
> -				out_sg->offset = 0;
>  			}
>  			sg_dma_address(out_sg) = 0;
>  			sg_dma_len(out_sg) = 0;
> -- 
> 2.34.1

On 20/03/25 07:16, Andrew Morton wrote:
> On Wed, 19 Mar 2025 16:44:38 +0530 T Pratham <t-pratham@ti.com> wrote:
>
>> The split_sg_phys function was incorrectly setting the offsets of all
>> scatterlist entries (except the first) to 0. Only the first scatterlist
>> entry's offset and length needs to be modified to account for the skip.
>> Setting the rest entries' offsets to 0 could lead to incorrect data
>> access.
>>
>> This patch removes the offending code, ensuring that the page offsets
>> in the input scatterlist are preserved in the output scatterlist.
> Is this merely from code inspection, or is this issues known to have
> observable runtime effects?
>
> If the latter, please provide a complete description.

Hi,

I am using this function in a crypto driver that I'm currently
developing (not yet sent to mailing list). During testing, it was
observed that the output scatterlists (except the first one) contained
incorrect garbage data.

I narrowed this issue down to the call of sg_split(). Upon debugging
inside this function, I found that this resetting of offset is the cause
of the problem, causing the subsequent scatterlists to point to
incorrect memory locations in a page. By removing this code, I am
obtaining expected data in all the split output scatterlists. Thus, this
was indeed causing observable runtime effects!

Regards
T Pratham <t-pratham@ti.com>

>> ...
>>
>> --- a/lib/sg_split.c
>> +++ b/lib/sg_split.c
>> @@ -88,8 +88,6 @@ static void sg_split_phys(struct sg_splitter *splitters, const int nb_splits)
>>  			if (!j) {
>>  				out_sg->offset += split->skip_sg0;
>>  				out_sg->length -= split->skip_sg0;
>> -			} else {
>> -				out_sg->offset = 0;
>>  			}
>>  			sg_dma_address(out_sg) = 0;
>>  			sg_dma_len(out_sg) = 0;
>> -- 
>> 2.34.1

On 20/03/25 10:28, T Pratham wrote:
> On 20/03/25 07:16, Andrew Morton wrote:
>> On Wed, 19 Mar 2025 16:44:38 +0530 T Pratham <t-pratham@ti.com> wrote:
>>
>>> The split_sg_phys function was incorrectly setting the offsets of all
>>> scatterlist entries (except the first) to 0. Only the first scatterlist
>>> entry's offset and length needs to be modified to account for the skip.
>>> Setting the rest entries' offsets to 0 could lead to incorrect data
>>> access.
>>>
>>> This patch removes the offending code, ensuring that the page offsets
>>> in the input scatterlist are preserved in the output scatterlist.
>> Is this merely from code inspection, or is this issues known to have
>> observable runtime effects?
>>
>> If the latter, please provide a complete description.
> Hi,
>
> I am using this function in a crypto driver that I'm currently
> developing (not yet sent to mailing list). During testing, it was
> observed that the output scatterlists (except the first one) contained
> incorrect garbage data.
>
> I narrowed this issue down to the call of sg_split(). Upon debugging
> inside this function, I found that this resetting of offset is the cause
> of the problem, causing the subsequent scatterlists to point to
> incorrect memory locations in a page. By removing this code, I am
> obtaining expected data in all the split output scatterlists. Thus, this
> was indeed causing observable runtime effects!
>
> Regards
> T Pratham <t-pratham@ti.com>

Hi Andrew,

Do you need the above details to be incorporated into the commit message
and be resent? Kindly let me know.

Regards
T Pratham <t-pratham@ti.com>

>>> ...
>>>
>>> --- a/lib/sg_split.c
>>> +++ b/lib/sg_split.c
>>> @@ -88,8 +88,6 @@ static void sg_split_phys(struct sg_splitter *splitters, const int nb_splits)
>>>  			if (!j) {
>>>  				out_sg->offset += split->skip_sg0;
>>>  				out_sg->length -= split->skip_sg0;
>>> -			} else {
>>> -				out_sg->offset = 0;
>>>  			}
>>>  			sg_dma_address(out_sg) = 0;
>>>  			sg_dma_len(out_sg) = 0;
>>> -- 
>>> 2.34.1

On Wed, 26 Mar 2025 14:13:02 +0530 T Pratham <t-pratham@ti.com> wrote:

> >> Is this merely from code inspection, or is this issues known to have
> >> observable runtime effects?
> >>
> >> If the latter, please provide a complete description.
> > Hi,
> >
> > I am using this function in a crypto driver that I'm currently
> > developing (not yet sent to mailing list). During testing, it was
> > observed that the output scatterlists (except the first one) contained
> > incorrect garbage data.
> >
> > I narrowed this issue down to the call of sg_split(). Upon debugging
> > inside this function, I found that this resetting of offset is the cause
> > of the problem, causing the subsequent scatterlists to point to
> > incorrect memory locations in a page. By removing this code, I am
> > obtaining expected data in all the split output scatterlists. Thus, this
> > was indeed causing observable runtime effects!
> >
> > Regards
> > T Pratham <t-pratham@ti.com>
> 
> Hi Andrew,
> 
> Do you need the above details to be incorporated into the commit message
> and be resent? Kindly let me know.

I pasted it into the changelog.

I also added cc:stable, as this might be affecting other drivers,
whether in-tree or out-of-tree.