From: Eric Dumazet <edumazet@google.com>
To: Simon Kapadia <szymon@kapadia.pl>
Cc: security@kernel.org, Tom M0LTE <tom@m0lte.uk>,
linux-hams@vger.kernel.org
Subject: Re: Bug in the net/rom kernel module
Date: Wed, 24 May 2023 14:19:57 +0200 [thread overview]
Message-ID: <CANn89iK0sw_uR1VruzLbMP5hsujkMbgvLPA1kNiE9xOgqTiLog@mail.gmail.com> (raw)
In-Reply-To: <CAJE3E7Lb6aQMTGMQ4L63A=EMtYGqpsymW68nWc=nmGnJX-WKHQ@mail.gmail.com>
On Wed, May 24, 2023 at 2:03 PM Simon Kapadia <szymon@kapadia.pl> wrote:
>
> Hi Eric,
>
> I can confirm after testing it out today that your patch does indeed behave better than my simplified one -- only the data needed is sent out onto the network, packets are split out perfectly as expected, and everything seems to work correctly.
>
> Many thanks for your help, please go ahead and submit the patch :-)
Sure thing, thanks for double checking.
>
> -simon
>
> 26 144.322099355 M0GZP-2 → GB7IOW AX.25-NoL3 77 Text
>
>
> 0000 00 8e 84 6e 92 9e ae e0 9a 60 8e b4 a0 40 65 82 ...n.....`...@e.
>
> 0010 f0 54 68 69 73 20 69 73 20 61 20 74 65 73 74 20 .This is a test
>
> 0020 74 6f 20 73 65 65 20 68 6f 77 20 6c 6f 6e 67 65 to see how longe
>
> 0030 72 20 6d 65 73 73 61 67 65 73 20 77 69 6c 6c 20 r messages will
>
> 0040 62 65 20 73 70 6c 69 74 20 75 70 20 6f be split up o
>
>
> 27 144.322396807 M0GZP-2 → GB7IOW AX.25-NoL3 61 Text
>
>
> 0000 00 8e 84 6e 92 9e ae e0 9a 60 8e b4 a0 40 65 94 ...n.....`...@e.
>
> 0010 f0 76 65 72 20 6d 75 6c 74 69 70 6c 65 20 70 61 .ver multiple pa
>
> 0020 63 6b 65 74 73 2c 20 61 73 73 75 6d 69 6e 67 20 ckets, assuming
>
> 0030 74 68 61 74 20 68 61 70 70 65 6e 73 0d that happens.
>
>
>
> --
> Simon Kapadia, B.A., M.Sc., C.Eng.
> szymon@kapadia.pl | http://kapadia.pl
> http://astrophotos.uk/
>
>
> On Tue, 23 May 2023 at 16:04, Simon Kapadia <szymon@kapadia.pl> wrote:
>>
>> Sorry for the late reply, am doing this in the background while working. Sure, that makes sense -- includes the NETWORK_LEN in the alloc_skb but doesn't dump it onto the put. I've rebuilt my box since setting this up last so am going to have to recompile (and that takes a good long time on a pi), I'll try it out hopefully this evening when I get it all compiled but I think your patch is indeed "more correct" :-)
>>
>> -simon
>>
>> --
>> Simon Kapadia, B.A., M.Sc., C.Eng.
>> szymon@kapadia.pl | http://kapadia.pl
>> http://astrophotos.uk/
>>
>>
>> On Tue, 23 May 2023 at 12:38, Eric Dumazet <edumazet@google.com> wrote:
>>>
>>> On Tue, May 23, 2023 at 1:32 PM Eric Dumazet <edumazet@google.com> wrote:
>>> >
>>> > On Tue, May 23, 2023 at 12:47 PM Simon Kapadia <szymon@kapadia.pl> wrote:
>>> > >
>>> > > Would be much appreciated if you could send a patch -- happy to have my name to it, just haven't done so in this century so would likely make a stylistic error or something stupid!
>>> > >
>>> > > Thanks,
>>> > >
>>> > > -simon
>>> > >
>>> > > --- linux/net/netrom/nr_subr.c.orig 2023-04-19 19:50:03.080791821 +0100
>>> > > +++ linux/net/netrom/nr_subr.c 2023-04-19 19:50:22.410995440 +0100
>>> > > @@ -149,7 +149,7 @@ void nr_write_internal(struct sock *sk,
>>> > > */
>>> > > skb_reserve(skb, NR_NETWORK_LEN);
>>> > >
>>> > > - dptr = skb_put(skb, skb_tailroom(skb));
>>> > > + dptr = skb_put(skb, len);
>>> >
>>> >
>>> > Note that this patch does not look correct to me.
>>> >
>>> > len includes NR_NETWORK_LEN at this point.
>>> >
>>> > nr_transmit_buffer() will later add:
>>> >
>>> > dptr = skb_push(skb, NR_NETWORK_LEN);
>>> >
>>> > If you look again at your pcap after your patch, I guess we still send
>>> > 15 extra bytes ?
>>>
>>> I would think the following patch is more correct .
>>>
>>> diff --git a/net/netrom/nr_subr.c b/net/netrom/nr_subr.c
>>> index 3f99b432ea707e20a9620fb89cdf37d5e4f121e9..e2d2af924cff4a4103e59e04a6efe69c6fcca23e
>>> 100644
>>> --- a/net/netrom/nr_subr.c
>>> +++ b/net/netrom/nr_subr.c
>>> @@ -123,7 +123,7 @@ void nr_write_internal(struct sock *sk, int frametype)
>>> unsigned char *dptr;
>>> int len, timeout;
>>>
>>> - len = NR_NETWORK_LEN + NR_TRANSPORT_LEN;
>>> + len = NR_TRANSPORT_LEN;
>>>
>>> switch (frametype & 0x0F) {
>>> case NR_CONNREQ:
>>> @@ -141,7 +141,8 @@ void nr_write_internal(struct sock *sk, int frametype)
>>> return;
>>> }
>>>
>>> - if ((skb = alloc_skb(len, GFP_ATOMIC)) == NULL)
>>> + skb = alloc_skb(NR_NETWORK_LEN + len, GFP_ATOMIC);
>>> + if (!skb)
>>> return;
>>>
>>> /*
>>> @@ -149,7 +150,7 @@ void nr_write_internal(struct sock *sk, int frametype)
>>> */
>>> skb_reserve(skb, NR_NETWORK_LEN);
>>>
>>> - dptr = skb_put(skb, skb_tailroom(skb));
>>> + dptr = skb_put(skb, len);
>>>
>>> switch (frametype & 0x0F) {
>>> case NR_CONNREQ:
prev parent reply other threads:[~2023-05-24 12:19 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CAJE3E7JoJfrnST3dU-vu_=6T_eB5gWVBKbfUSkCMu44gkbD3iQ@mail.gmail.com>
[not found] ` <CAJE3E7LfO5us9MvKZX1MMP8mUTFzO324cxshHTDSw6qkB6kHTw@mail.gmail.com>
[not found] ` <CANn89iKEX9wLYA3aisvVAGKJ2mGr9K35ALZTwHeM7DwEV2+nkg@mail.gmail.com>
[not found] ` <CAJE3E7+EZVLRoLohp3Y2529HpKSndFZAyakX-FcbEe9wMBe-Bg@mail.gmail.com>
2023-05-23 11:32 ` Bug in the net/rom kernel module Eric Dumazet
2023-05-23 11:38 ` Eric Dumazet
[not found] ` <CAJE3E7+5ox6nx=j+6W4brDnzfKQVMdUiBi0ieFohQDNaWXJxEw@mail.gmail.com>
[not found] ` <CAJE3E7Lb6aQMTGMQ4L63A=EMtYGqpsymW68nWc=nmGnJX-WKHQ@mail.gmail.com>
2023-05-24 12:19 ` Eric Dumazet [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANn89iK0sw_uR1VruzLbMP5hsujkMbgvLPA1kNiE9xOgqTiLog@mail.gmail.com \
--to=edumazet@google.com \
--cc=linux-hams@vger.kernel.org \
--cc=security@kernel.org \
--cc=szymon@kapadia.pl \
--cc=tom@m0lte.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).