[12/12] io/channel-socket: qio_channel_socket_flush(): improve msg validation

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 25, 2023, 7:40 p.m. UTC

Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
---
 io/channel-socket.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Maksim Davydov Sept. 26, 2023, 9:04 a.m. UTC | #1

Could you add a comment into the commit message why ee_data must be
bigger than ee_info?

On 9/25/23 22:40, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> ---
>   io/channel-socket.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 02ffb51e99..3a899b0608 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -782,6 +782,11 @@ static int qio_channel_socket_flush(QIOChannel *ioc,
>                                "Error not from zero copy");
>               return -1;
>           }
> +        if (serr->ee_data < serr->ee_info) {
> +            error_setg_errno(errp, serr->ee_origin,
> +                             "Wrong notification bounds");
> +            return -1;
> +        }
>   
>           /* No errors, count successfully finished sendmsg()*/
>           sioc->zero_copy_sent += serr->ee_data - serr->ee_info + 1;

Vladimir Sementsov-Ogievskiy Sept. 26, 2023, 10:19 a.m. UTC | #2

On 26.09.23 12:04, Maksim Davydov wrote:
> Could you add a comment into the commit message why ee_data must be
> bigger than ee_info?

As I understand, in this case ee_data is lower bound and ee_info is upper bound of notification:

https://docs.kernel.org/networking/msg_zerocopy.html#notification-parsing

and the next line "sioc->zero_copy_sent += serr->ee_data - serr->ee_info + 1;" actually depends on it.

So, I'll add:

For SO_EE_ORIGIN_ZEROCOPY the 32-bit notification range is encoded
as [ee_info, ee_data] inclusively, so ee_info should be less or
equal to ee_data.

> 
> On 9/25/23 22:40, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
>> ---
>>   io/channel-socket.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/io/channel-socket.c b/io/channel-socket.c
>> index 02ffb51e99..3a899b0608 100644
>> --- a/io/channel-socket.c
>> +++ b/io/channel-socket.c
>> @@ -782,6 +782,11 @@ static int qio_channel_socket_flush(QIOChannel *ioc,
>>                                "Error not from zero copy");
>>               return -1;
>>           }
>> +        if (serr->ee_data < serr->ee_info) {
>> +            error_setg_errno(errp, serr->ee_origin,
>> +                             "Wrong notification bounds");
>> +            return -1;
>> +        }
>>           /* No errors, count successfully finished sendmsg()*/
>>           sioc->zero_copy_sent += serr->ee_data - serr->ee_info + 1;
>

Patch

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 02ffb51e99..3a899b0608 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -782,6 +782,11 @@  static int qio_channel_socket_flush(QIOChannel *ioc,
                              "Error not from zero copy");
             return -1;
         }
+        if (serr->ee_data < serr->ee_info) {
+            error_setg_errno(errp, serr->ee_origin,
+                             "Wrong notification bounds");
+            return -1;
+        }
 
         /* No errors, count successfully finished sendmsg()*/
         sioc->zero_copy_sent += serr->ee_data - serr->ee_info + 1;