XDMA crashes on zero-length sg entries

XDMA crashes on zero-length sg entries

[Martin Tůma]

Hi,
The Xilinx XDMA driver crashes when the scatterlist provided to
xdma_prep_device_sg() contains an empty entry, i.e. sg_dma_len()
returns zero. As I do get such sgl from v4l2 I suppose that this
is a valid scenario and not a bug in our parent mgb4 driver. Also
the documentation for sg_dma_len() suggests that there may be
zero-length entries.

The following trivial patch fixes the crash:

diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
index 462109c61653..cd5fcd911c50 100644
--- a/drivers/dma/xilinx/xdma.c
+++ b/drivers/dma/xilinx/xdma.c
@@ -487,6 +487,8 @@ xdma_prep_device_sg(struct dma_chan *chan, struct 
scatterlist *sgl,
         for_each_sg(sgl, sg, sg_len, i) {
                 addr = sg_dma_address(sg);
                 rest = sg_dma_len(sg);
+               if (!rest)
+                       break;

                 do {
                         len = min_t(u32, rest, XDMA_DESC_BLEN_MAX);

M.

Re: XDMA crashes on zero-length sg entries

[Lizhi Hou]

Hi Martin,

Based on the dmaengine document

https://kernel.org/doc/html/v6.3-rc2/driver-api/dmaengine/client.html

The sg_len used for dmaengine_prep_slave_sg should be returned by 
dma_map_sg.

This implies there should not be zero segment for the first sg_len segments.

I think that is why we need to provide 'sg_len' for 
dmaengine_prep_slave_sg().

With 'sg_len', we do not need to worry about zero length sg in the 
driver callback.


Thanks,

Lizhi

On 3/16/23 10:03, Martin Tůma wrote:
> Hi,
> The Xilinx XDMA driver crashes when the scatterlist provided to
> xdma_prep_device_sg() contains an empty entry, i.e. sg_dma_len()
> returns zero. As I do get such sgl from v4l2 I suppose that this
> is a valid scenario and not a bug in our parent mgb4 driver. Also
> the documentation for sg_dma_len() suggests that there may be
> zero-length entries.
>
> The following trivial patch fixes the crash:
>
> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
> index 462109c61653..cd5fcd911c50 100644
> --- a/drivers/dma/xilinx/xdma.c
> +++ b/drivers/dma/xilinx/xdma.c
> @@ -487,6 +487,8 @@ xdma_prep_device_sg(struct dma_chan *chan, struct 
> scatterlist *sgl,
>         for_each_sg(sgl, sg, sg_len, i) {
>                 addr = sg_dma_address(sg);
>                 rest = sg_dma_len(sg);
> +               if (!rest)
> +                       break;
>
>                 do {
>                         len = min_t(u32, rest, XDMA_DESC_BLEN_MAX);
>
> M.

Re: XDMA crashes on zero-length sg entries

[Martin Tůma]

Hi

On 16. 03. 23 19:25, Lizhi Hou wrote:
> Hi Martin,
> 
> Based on the dmaengine document
> 
> https://kernel.org/doc/html/v6.3-rc2/driver-api/dmaengine/client.html
> 
> The sg_len used for dmaengine_prep_slave_sg should be returned by 
> dma_map_sg.
> 
> This implies there should not be zero segment for the first sg_len 
> segments.
> 

I do get the mapping from vb2_dma_sg_plane_desc() which internally
uses dma_map_sg_attrs() and yet in some special cases sg_dma_len()
returns zero. So this is a "real-life" scenario. I'm still investigating
what the root cause is for getting such empty mappings from v4l2, but
the DMA driver should IMHO not crash the kernel in such case anyway.
I have looked into some randomly chosen kernel code that uses
sg_dma_len() and all usages that I have seen are prepared for the case
the length is zero. All but one driver simply stop the iteration while
the remaining one reported this case as an error.

M.

> I think that is why we need to provide 'sg_len' for 
> dmaengine_prep_slave_sg().
> 
> With 'sg_len', we do not need to worry about zero length sg in the 
> driver callback.
> 
> 
> Thanks,
> 
> Lizhi
> 
> On 3/16/23 10:03, Martin Tůma wrote:
>> Hi,
>> The Xilinx XDMA driver crashes when the scatterlist provided to
>> xdma_prep_device_sg() contains an empty entry, i.e. sg_dma_len()
>> returns zero. As I do get such sgl from v4l2 I suppose that this
>> is a valid scenario and not a bug in our parent mgb4 driver. Also
>> the documentation for sg_dma_len() suggests that there may be
>> zero-length entries.
>>
>> The following trivial patch fixes the crash:
>>
>> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
>> index 462109c61653..cd5fcd911c50 100644
>> --- a/drivers/dma/xilinx/xdma.c
>> +++ b/drivers/dma/xilinx/xdma.c
>> @@ -487,6 +487,8 @@ xdma_prep_device_sg(struct dma_chan *chan, struct 
>> scatterlist *sgl,
>>         for_each_sg(sgl, sg, sg_len, i) {
>>                 addr = sg_dma_address(sg);
>>                 rest = sg_dma_len(sg);
>> +               if (!rest)
>> +                       break;
>>
>>                 do {
>>                         len = min_t(u32, rest, XDMA_DESC_BLEN_MAX);
>>
>> M.

Re: XDMA crashes on zero-length sg entries

[Lizhi Hou]

Hi Martin,

On 3/17/23 04:18, Martin Tůma wrote:
> Hi
>
> On 16. 03. 23 19:25, Lizhi Hou wrote:
>> Hi Martin,
>>
>> Based on the dmaengine document
>>
>> https://kernel.org/doc/html/v6.3-rc2/driver-api/dmaengine/client.html
>>
>> The sg_len used for dmaengine_prep_slave_sg should be returned by 
>> dma_map_sg.
>>
>> This implies there should not be zero segment for the first sg_len 
>> segments.
>>
>
> I do get the mapping from vb2_dma_sg_plane_desc() which internally
> uses dma_map_sg_attrs() and yet in some special cases sg_dma_len()
> returns zero. So this is a "real-life" scenario. I'm still investigating
This sounds odd to me. dma_map_sg() is supposed to return number of 
Non-zero sg because the zero length sg will be merged.
> what the root cause is for getting such empty mappings from v4l2, but
> the DMA driver should IMHO not crash the kernel in such case anyway.

I checked some dma driver and I did not see obvious code to check the sg 
len. e.g.

https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/dma/k3dma.c#L531

> I have looked into some randomly chosen kernel code that uses
> sg_dma_len() and all usages that I have seen are prepared for the case
> the length is zero. All but one driver simply stop the iteration while
> the remaining one reported this case as an error.

Are those dma drivers?

Adding a check sounds fine to me. And I am going to create a patch 
anyway.  :)


Thanks,

Lizhi

>
> M.
>
>> I think that is why we need to provide 'sg_len' for 
>> dmaengine_prep_slave_sg().
>>
>> With 'sg_len', we do not need to worry about zero length sg in the 
>> driver callback.
>>
>>
>> Thanks,
>>
>> Lizhi
>>
>> On 3/16/23 10:03, Martin Tůma wrote:
>>> Hi,
>>> The Xilinx XDMA driver crashes when the scatterlist provided to
>>> xdma_prep_device_sg() contains an empty entry, i.e. sg_dma_len()
>>> returns zero. As I do get such sgl from v4l2 I suppose that this
>>> is a valid scenario and not a bug in our parent mgb4 driver. Also
>>> the documentation for sg_dma_len() suggests that there may be
>>> zero-length entries.
>>>
>>> The following trivial patch fixes the crash:
>>>
>>> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
>>> index 462109c61653..cd5fcd911c50 100644
>>> --- a/drivers/dma/xilinx/xdma.c
>>> +++ b/drivers/dma/xilinx/xdma.c
>>> @@ -487,6 +487,8 @@ xdma_prep_device_sg(struct dma_chan *chan, 
>>> struct scatterlist *sgl,
>>>         for_each_sg(sgl, sg, sg_len, i) {
>>>                 addr = sg_dma_address(sg);
>>>                 rest = sg_dma_len(sg);
>>> +               if (!rest)
>>> +                       break;
>>>
>>>                 do {
>>>                         len = min_t(u32, rest, XDMA_DESC_BLEN_MAX);
>>>
>>> M.
>

Re: XDMA crashes on zero-length sg entries

[Martin Tůma]

Hi,

On 17. 03. 23 18:19, Lizhi Hou wrote:
> Hi Martin,
> 
> On 3/17/23 04:18, Martin Tůma wrote:
>> Hi
>>
>> On 16. 03. 23 19:25, Lizhi Hou wrote:
>>> Hi Martin,
>>>
>>> Based on the dmaengine document
>>>
>>> https://kernel.org/doc/html/v6.3-rc2/driver-api/dmaengine/client.html
>>>
>>> The sg_len used for dmaengine_prep_slave_sg should be returned by 
>>> dma_map_sg.
>>>
>>> This implies there should not be zero segment for the first sg_len 
>>> segments.
>>>
>>
>> I do get the mapping from vb2_dma_sg_plane_desc() which internally
>> uses dma_map_sg_attrs() and yet in some special cases sg_dma_len()
>> returns zero. So this is a "real-life" scenario. I'm still investigating
> This sounds odd to me. dma_map_sg() is supposed to return number of 
> Non-zero sg because the zero length sg will be merged.
>> what the root cause is for getting such empty mappings from v4l2, but
>> the DMA driver should IMHO not crash the kernel in such case anyway.
> 
> I checked some dma driver and I did not see obvious code to check the sg 
> len. e.g.
> 
> https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/dma/k3dma.c#L531
> 

Didn't examinate this one in detail, but I don't think this one would
crash either. In your case, the problem is, that you do not pre-allocate
the descriptor in case sg_dma_len(sg) is zero and then access it in:

desc->bytes = cpu_to_le32(len);

which is the exact line where xdma crashes.

M.

>> I have looked into some randomly chosen kernel code that uses
>> sg_dma_len() and all usages that I have seen are prepared for the case
>> the length is zero. All but one driver simply stop the iteration while
>> the remaining one reported this case as an error.
> 
> Are those dma drivers?
> 
> Adding a check sounds fine to me. And I am going to create a patch 
> anyway.  :)
> 
> 
> Thanks,
> 
> Lizhi
> 
>>
>> M.
>>
>>> I think that is why we need to provide 'sg_len' for 
>>> dmaengine_prep_slave_sg().
>>>
>>> With 'sg_len', we do not need to worry about zero length sg in the 
>>> driver callback.
>>>
>>>
>>> Thanks,
>>>
>>> Lizhi
>>>
>>> On 3/16/23 10:03, Martin Tůma wrote:
>>>> Hi,
>>>> The Xilinx XDMA driver crashes when the scatterlist provided to
>>>> xdma_prep_device_sg() contains an empty entry, i.e. sg_dma_len()
>>>> returns zero. As I do get such sgl from v4l2 I suppose that this
>>>> is a valid scenario and not a bug in our parent mgb4 driver. Also
>>>> the documentation for sg_dma_len() suggests that there may be
>>>> zero-length entries.
>>>>
>>>> The following trivial patch fixes the crash:
>>>>
>>>> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
>>>> index 462109c61653..cd5fcd911c50 100644
>>>> --- a/drivers/dma/xilinx/xdma.c
>>>> +++ b/drivers/dma/xilinx/xdma.c
>>>> @@ -487,6 +487,8 @@ xdma_prep_device_sg(struct dma_chan *chan, 
>>>> struct scatterlist *sgl,
>>>>         for_each_sg(sgl, sg, sg_len, i) {
>>>>                 addr = sg_dma_address(sg);
>>>>                 rest = sg_dma_len(sg);
>>>> +               if (!rest)
>>>> +                       break;
>>>>
>>>>                 do {
>>>>                         len = min_t(u32, rest, XDMA_DESC_BLEN_MAX);
>>>>
>>>> M.
>>