kernel BUG in __skb_gso_segment

kernel BUG in __skb_gso_segment

[Tudor Ambarus]

Hi,

There's a bug [1] reported by syzkaller in linux-5.15.y that I'd like
to squash. The commit in stable that introduces the bug is:
b99c71f90978 net: skip virtio_net_hdr_set_proto if protocol already set
The upstream commit for this is:
1ed1d592113959f00cc552c3b9f47ca2d157768f

I discovered that in mainline this bug was squashed by the following
commits:
e9d3f80935b6 ("net/af_packet: make sure to pull mac header")
dfed913e8b55 ("net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO")

I'm seeking for some guidance on how to fix linux-5.15.y. From what I
understand, the bug in stable is triggered because we end up with a
header offset of 18, that eventually triggers the GSO crash in
__skb_pull. If I revert the commit in culprit from linux-5.15.y, we'll
end up with a header offset of 14, the bug is not hit and the packet is 
dropped at validate_xmit_skb() time. I'm wondering if reverting it is
the right thing to do, as the commit is marked as a fix. Backporting the
2 commits from mainline is not an option as they introduce new support.
Would such a patch be better than reverting the offending commit?

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index a960de68ac69..188b6f05e5bd 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -25,7 +25,7 @@ static inline bool virtio_net_hdr_match_proto(__be16 
protocol, __u8 gso_type)
  static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
                                            const struct virtio_net_hdr 
*hdr)
  {
-       if (skb->protocol)
+       if (skb->protocol && skb->protocol != htons(ETH_P_ALL))
                 return 0;

         switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {


Thanks!
ta

[1] 
https://syzkaller.appspot.com/bug?id=ce5575575f074c33ff80d104f5baee26f22e95f5

Re: kernel BUG in __skb_gso_segment

[Willem de Bruijn]

On Tue, Dec 20, 2022 at 8:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi,
>
> There's a bug [1] reported by syzkaller in linux-5.15.y that I'd like
> to squash. The commit in stable that introduces the bug is:
> b99c71f90978 net: skip virtio_net_hdr_set_proto if protocol already set
> The upstream commit for this is:
> 1ed1d592113959f00cc552c3b9f47ca2d157768f
>
> I discovered that in mainline this bug was squashed by the following
> commits:
> e9d3f80935b6 ("net/af_packet: make sure to pull mac header")
> dfed913e8b55 ("net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO")
>
> I'm seeking for some guidance on how to fix linux-5.15.y. From what I
> understand, the bug in stable is triggered because we end up with a
> header offset of 18, that eventually triggers the GSO crash in
> __skb_pull. If I revert the commit in culprit from linux-5.15.y, we'll
> end up with a header offset of 14, the bug is not hit and the packet is
> dropped at validate_xmit_skb() time. I'm wondering if reverting it is
> the right thing to do, as the commit is marked as a fix. Backporting the
> 2 commits from mainline is not an option as they introduce new support.
> Would such a patch be better than reverting the offending commit?

If both patches can be backported without conflicts, in this case I
think that is the preferred solution.

If the fix were obvious that would be an option. But the history for
this code indicates that it isn't. It has a history of fixes for edge
cases.

Backporting the two avoids a fork that would make backporting
additional fixes harder. The first of the two is technically not a
fix, but evidently together they are for this case. And the additional
logic and risk backported seems manageable.

Admittedly that is subjective. I can help take a closer look at a
custom fix if consensus is that is preferable.

Re: kernel BUG in __skb_gso_segment

[Tudor Ambarus]

Hi,

I added Greg KH to the thread, maybe he can shed some light on whether
new support can be marked as fixes and backported to stable. The rules
on what kind of patches are accepted into the -stable tree don't mention
new support:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

On 20.12.2022 20:27, Willem de Bruijn wrote:
> On Tue, Dec 20, 2022 at 8:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Hi,
>>
>> There's a bug [1] reported by syzkaller in linux-5.15.y that I'd like
>> to squash. The commit in stable that introduces the bug is:
>> b99c71f90978 net: skip virtio_net_hdr_set_proto if protocol already set
>> The upstream commit for this is:
>> 1ed1d592113959f00cc552c3b9f47ca2d157768f
>>
>> I discovered that in mainline this bug was squashed by the following
>> commits:
>> e9d3f80935b6 ("net/af_packet: make sure to pull mac header")
>> dfed913e8b55 ("net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO")
>>
>> I'm seeking for some guidance on how to fix linux-5.15.y. From what I
>> understand, the bug in stable is triggered because we end up with a
>> header offset of 18, that eventually triggers the GSO crash in
>> __skb_pull. If I revert the commit in culprit from linux-5.15.y, we'll
>> end up with a header offset of 14, the bug is not hit and the packet is
>> dropped at validate_xmit_skb() time. I'm wondering if reverting it is
>> the right thing to do, as the commit is marked as a fix. Backporting the
>> 2 commits from mainline is not an option as they introduce new support.
>> Would such a patch be better than reverting the offending commit?
> 
> If both patches can be backported without conflicts, in this case I
> think that is the preferred solution.

I confirm both patches can be backported without conflicts.

> 
> If the fix were obvious that would be an option. But the history for
> this code indicates that it isn't. It has a history of fixes for edge
> cases.
> 
> Backporting the two avoids a fork that would make backporting
> additional fixes harder. The first of the two is technically not a

I agree that a fork would make backporting additional fixes harder.
I'm no networking guy, but I can debug further to understand whether the
patch that I proposed or other would make sense for both mainline and
stable kernels. We'll avoid the fork this way.

> fix, but evidently together they are for this case. And the additional
> logic and risk backported seems manageable.

It is, indeed.

> 
> Admittedly that is subjective. I can help take a closer look at a
> custom fix if consensus is that is preferable.

Thanks. Let's wait for others to comment so that we have an agreement.

Cheers,
ta

Re: kernel BUG in __skb_gso_segment

[Greg KH]

On Wed, Dec 21, 2022 at 09:28:16AM +0200, Tudor Ambarus wrote:
> Hi,
> 
> I added Greg KH to the thread, maybe he can shed some light on whether
> new support can be marked as fixes and backported to stable. The rules
> on what kind of patches are accepted into the -stable tree don't mention
> new support:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

As you say, we don't take new features into older kernels.  Unless they
fix a reported problem, if so, submit the git ids to us and we will be
glad to review them.

thanks,

greg k-h

Re: kernel BUG in __skb_gso_segment

[Tudor Ambarus]

On 21.12.2022 09:37, Greg KH wrote:
> On Wed, Dec 21, 2022 at 09:28:16AM +0200, Tudor Ambarus wrote:
>> Hi,
>>
>> I added Greg KH to the thread, maybe he can shed some light on whether
>> new support can be marked as fixes and backported to stable. The rules
>> on what kind of patches are accepted into the -stable tree don't mention
>> new support:
>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> 
> As you say, we don't take new features into older kernels.  Unless they
> fix a reported problem, if so, submit the git ids to us and we will be
> glad to review them.
> 

They do fix a bug. I'm taking care of it. Shall I update
Documentation/process/stable-kernel-rules.rst to mention this rule as
well?


Thanks,
ta

Re: kernel BUG in __skb_gso_segment

[Greg KH]

On Wed, Dec 21, 2022 at 09:42:59AM +0200, Tudor Ambarus wrote:
> 
> 
> On 21.12.2022 09:37, Greg KH wrote:
> > On Wed, Dec 21, 2022 at 09:28:16AM +0200, Tudor Ambarus wrote:
> > > Hi,
> > > 
> > > I added Greg KH to the thread, maybe he can shed some light on whether
> > > new support can be marked as fixes and backported to stable. The rules
> > > on what kind of patches are accepted into the -stable tree don't mention
> > > new support:
> > > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > 
> > As you say, we don't take new features into older kernels.  Unless they
> > fix a reported problem, if so, submit the git ids to us and we will be
> > glad to review them.
> > 
> 
> They do fix a bug. I'm taking care of it. Shall I update
> Documentation/process/stable-kernel-rules.rst to mention this rule as
> well?

How exactly would you change it, and why?

Re: kernel BUG in __skb_gso_segment

[Tudor Ambarus]

On 21.12.2022 14:41, Greg KH wrote:
> On Wed, Dec 21, 2022 at 09:42:59AM +0200, Tudor Ambarus wrote:
>>
>>
>> On 21.12.2022 09:37, Greg KH wrote:
>>> On Wed, Dec 21, 2022 at 09:28:16AM +0200, Tudor Ambarus wrote:
>>>> Hi,
>>>>
>>>> I added Greg KH to the thread, maybe he can shed some light on whether
>>>> new support can be marked as fixes and backported to stable. The rules
>>>> on what kind of patches are accepted into the -stable tree don't mention
>>>> new support:
>>>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>>>
>>> As you say, we don't take new features into older kernels.  Unless they
>>> fix a reported problem, if so, submit the git ids to us and we will be
>>> glad to review them.
>>>
>>
>> They do fix a bug. I'm taking care of it. Shall I update
>> Documentation/process/stable-kernel-rules.rst to mention this rule as
>> well?
> 
> How exactly would you change it, and why?

Something like this:

https://lore.kernel.org/linux-kernel/20221222091658.1975240-1-tudor.ambarus@linaro.org/T/#u

Cheers,
ta