[PATCH 0/2] util/iov: Do not assert offset is in iov

[PATCH 0/2] util/iov: Do not assert offset is in iov

[Akihiko Odaki]

iov_from_buf(), iov_to_buf(), iov_memset(), and iov_copy() asserts
that the given offset fits in the iov while tolerating the specified
number of bytes to operate with to be greater than the size of iov.
This is inconsistent so remove the assertions.

Asserting the offset fits in the iov makes sense if it is expected that
there are other operations that process the content before the offset
and the content is processed in order. Under this expectation, the
offset should point to the end of bytes that are previously processed
and fit in the iov. However, this expectation depends on the details of
the caller, and did not hold true at least one case and required code to
check iov_size(), which is added with commit 83ddb3dbba2e
("hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()").

Adding such a check is inefficient and error-prone. These functions
already tolerate the specified number of bytes to operate with to be
greater than the size of iov to avoid such checks so remove the
assertions to tolerate invalid offset as well. They return the number of
bytes they operated with so their callers can still check the returned
value to ensure there are sufficient space at the given offset.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Akihiko Odaki (2):
      util/iov: Do not assert offset is in iov
      Revert "hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()"

 include/qemu/iov.h  | 5 +++--
 hw/net/net_tx_pkt.c | 4 ----
 util/iov.c          | 5 -----
 3 files changed, 3 insertions(+), 11 deletions(-)
---
base-commit: fd87be1dada5672f877e03c2ca8504458292c479
change-id: 20240428-iov-a317b02601dc

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH 1/2] util/iov: Do not assert offset is in iov

[Akihiko Odaki]

iov_from_buf(), iov_to_buf(), iov_memset(), and iov_copy() asserts
that the given offset fits in the iov while tolerating the specified
number of bytes to operate with to be greater than the size of iov.
This is inconsistent so remove the assertions.

Asserting the offset fits in the iov makes sense if it is expected that
there are other operations that process the content before the offset
and the content is processed in order. Under this expectation, the
offset should point to the end of bytes that are previously processed
and fit in the iov. However, this expectation depends on the details of
the caller, and did not hold true at least one case and required code to
check iov_size(), which is added with commit 83ddb3dbba2e
("hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()").

Adding such a check is inefficient and error-prone. These functions
already tolerate the specified number of bytes to operate with to be
greater than the size of iov to avoid such checks so remove the
assertions to tolerate invalid offset as well. They return the number of
bytes they operated with so their callers can still check the returned
value to ensure there are sufficient space at the given offset.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/qemu/iov.h | 5 +++--
 util/iov.c         | 5 -----
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index 63a1c01965d1..33548058d2ee 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -30,7 +30,7 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);
  * only part of data will be copied, up to the end of the iovec.
  * Number of bytes actually copied will be returned, which is
  *  min(bytes, iov_size(iov)-offset)
- * `Offset' must point to the inside of iovec.
+ * Returns 0 when `offset' points to the outside of iovec.
  */
 size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
                          size_t offset, const void *buf, size_t bytes);
@@ -66,11 +66,12 @@ iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
 /**
  * Set data bytes pointed out by iovec `iov' of size `iov_cnt' elements,
  * starting at byte offset `start', to value `fillc', repeating it
- * `bytes' number of times.  `Offset' must point to the inside of iovec.
+ * `bytes' number of times.
  * If `bytes' is large enough, only last bytes portion of iovec,
  * up to the end of it, will be filled with the specified value.
  * Function return actual number of bytes processed, which is
  * min(size, iov_size(iov) - offset).
+ * Returns 0 when `offset' points to the outside of iovec.
  */
 size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
                   size_t offset, int fillc, size_t bytes);
diff --git a/util/iov.c b/util/iov.c
index 7e73948f5e3d..a523b406b7f8 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -36,7 +36,6 @@ size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
             offset -= iov[i].iov_len;
         }
     }
-    assert(offset == 0);
     return done;
 }
 
@@ -55,7 +54,6 @@ size_t iov_to_buf_full(const struct iovec *iov, const unsigned int iov_cnt,
             offset -= iov[i].iov_len;
         }
     }
-    assert(offset == 0);
     return done;
 }
 
@@ -74,7 +72,6 @@ size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
             offset -= iov[i].iov_len;
         }
     }
-    assert(offset == 0);
     return done;
 }
 
@@ -266,7 +263,6 @@ unsigned iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
         bytes -= len;
         offset = 0;
     }
-    assert(offset == 0);
     return j;
 }
 
@@ -337,7 +333,6 @@ size_t qemu_iovec_concat_iov(QEMUIOVector *dst,
             soffset -= src_iov[i].iov_len;
         }
     }
-    assert(soffset == 0); /* offset beyond end of src */
 
     return done;
 }

-- 
2.44.0

Re: [PATCH 1/2] util/iov: Do not assert offset is in iov

[Philippe Mathieu-Daudé]

ping?

On 28/4/24 13:11, Akihiko Odaki wrote:
> iov_from_buf(), iov_to_buf(), iov_memset(), and iov_copy() asserts
> that the given offset fits in the iov while tolerating the specified
> number of bytes to operate with to be greater than the size of iov.
> This is inconsistent so remove the assertions.
> 
> Asserting the offset fits in the iov makes sense if it is expected that
> there are other operations that process the content before the offset
> and the content is processed in order. Under this expectation, the
> offset should point to the end of bytes that are previously processed
> and fit in the iov. However, this expectation depends on the details of
> the caller, and did not hold true at least one case and required code to
> check iov_size(), which is added with commit 83ddb3dbba2e
> ("hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()").
> 
> Adding such a check is inefficient and error-prone. These functions
> already tolerate the specified number of bytes to operate with to be
> greater than the size of iov to avoid such checks so remove the
> assertions to tolerate invalid offset as well. They return the number of
> bytes they operated with so their callers can still check the returned
> value to ensure there are sufficient space at the given offset.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   include/qemu/iov.h | 5 +++--
>   util/iov.c         | 5 -----
>   2 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> index 63a1c01965d1..33548058d2ee 100644
> --- a/include/qemu/iov.h
> +++ b/include/qemu/iov.h
> @@ -30,7 +30,7 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);
>    * only part of data will be copied, up to the end of the iovec.
>    * Number of bytes actually copied will be returned, which is
>    *  min(bytes, iov_size(iov)-offset)
> - * `Offset' must point to the inside of iovec.
> + * Returns 0 when `offset' points to the outside of iovec.
>    */
>   size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
>                            size_t offset, const void *buf, size_t bytes);
> @@ -66,11 +66,12 @@ iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
>   /**
>    * Set data bytes pointed out by iovec `iov' of size `iov_cnt' elements,
>    * starting at byte offset `start', to value `fillc', repeating it
> - * `bytes' number of times.  `Offset' must point to the inside of iovec.
> + * `bytes' number of times.
>    * If `bytes' is large enough, only last bytes portion of iovec,
>    * up to the end of it, will be filled with the specified value.
>    * Function return actual number of bytes processed, which is
>    * min(size, iov_size(iov) - offset).
> + * Returns 0 when `offset' points to the outside of iovec.
>    */
>   size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
>                     size_t offset, int fillc, size_t bytes);
> diff --git a/util/iov.c b/util/iov.c
> index 7e73948f5e3d..a523b406b7f8 100644
> --- a/util/iov.c
> +++ b/util/iov.c
> @@ -36,7 +36,6 @@ size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
>               offset -= iov[i].iov_len;
>           }
>       }
> -    assert(offset == 0);
>       return done;
>   }
>   
> @@ -55,7 +54,6 @@ size_t iov_to_buf_full(const struct iovec *iov, const unsigned int iov_cnt,
>               offset -= iov[i].iov_len;
>           }
>       }
> -    assert(offset == 0);
>       return done;
>   }
>   
> @@ -74,7 +72,6 @@ size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
>               offset -= iov[i].iov_len;
>           }
>       }
> -    assert(offset == 0);
>       return done;
>   }
>   
> @@ -266,7 +263,6 @@ unsigned iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
>           bytes -= len;
>           offset = 0;
>       }
> -    assert(offset == 0);
>       return j;
>   }
>   
> @@ -337,7 +333,6 @@ size_t qemu_iovec_concat_iov(QEMUIOVector *dst,
>               soffset -= src_iov[i].iov_len;
>           }
>       }
> -    assert(soffset == 0); /* offset beyond end of src */
>   
>       return done;
>   }
> 

Re: [PATCH 1/2] util/iov: Do not assert offset is in iov

[Akihiko Odaki]

Jason, can you pull this series?

Regards,
Akihiko Odaki

On 2024/05/08 23:51, Philippe Mathieu-Daudé wrote:
> ping?
> 
> On 28/4/24 13:11, Akihiko Odaki wrote:
>> iov_from_buf(), iov_to_buf(), iov_memset(), and iov_copy() asserts
>> that the given offset fits in the iov while tolerating the specified
>> number of bytes to operate with to be greater than the size of iov.
>> This is inconsistent so remove the assertions.
>>
>> Asserting the offset fits in the iov makes sense if it is expected that
>> there are other operations that process the content before the offset
>> and the content is processed in order. Under this expectation, the
>> offset should point to the end of bytes that are previously processed
>> and fit in the iov. However, this expectation depends on the details of
>> the caller, and did not hold true at least one case and required code to
>> check iov_size(), which is added with commit 83ddb3dbba2e
>> ("hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()").
>>
>> Adding such a check is inefficient and error-prone. These functions
>> already tolerate the specified number of bytes to operate with to be
>> greater than the size of iov to avoid such checks so remove the
>> assertions to tolerate invalid offset as well. They return the number of
>> bytes they operated with so their callers can still check the returned
>> value to ensure there are sufficient space at the given offset.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   include/qemu/iov.h | 5 +++--
>>   util/iov.c         | 5 -----
>>   2 files changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
>> index 63a1c01965d1..33548058d2ee 100644
>> --- a/include/qemu/iov.h
>> +++ b/include/qemu/iov.h
>> @@ -30,7 +30,7 @@ size_t iov_size(const struct iovec *iov, const 
>> unsigned int iov_cnt);
>>    * only part of data will be copied, up to the end of the iovec.
>>    * Number of bytes actually copied will be returned, which is
>>    *  min(bytes, iov_size(iov)-offset)
>> - * `Offset' must point to the inside of iovec.
>> + * Returns 0 when `offset' points to the outside of iovec.
>>    */
>>   size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
>>                            size_t offset, const void *buf, size_t bytes);
>> @@ -66,11 +66,12 @@ iov_to_buf(const struct iovec *iov, const unsigned 
>> int iov_cnt,
>>   /**
>>    * Set data bytes pointed out by iovec `iov' of size `iov_cnt' 
>> elements,
>>    * starting at byte offset `start', to value `fillc', repeating it
>> - * `bytes' number of times.  `Offset' must point to the inside of iovec.
>> + * `bytes' number of times.
>>    * If `bytes' is large enough, only last bytes portion of iovec,
>>    * up to the end of it, will be filled with the specified value.
>>    * Function return actual number of bytes processed, which is
>>    * min(size, iov_size(iov) - offset).
>> + * Returns 0 when `offset' points to the outside of iovec.
>>    */
>>   size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
>>                     size_t offset, int fillc, size_t bytes);
>> diff --git a/util/iov.c b/util/iov.c
>> index 7e73948f5e3d..a523b406b7f8 100644
>> --- a/util/iov.c
>> +++ b/util/iov.c
>> @@ -36,7 +36,6 @@ size_t iov_from_buf_full(const struct iovec *iov, 
>> unsigned int iov_cnt,
>>               offset -= iov[i].iov_len;
>>           }
>>       }
>> -    assert(offset == 0);
>>       return done;
>>   }
>> @@ -55,7 +54,6 @@ size_t iov_to_buf_full(const struct iovec *iov, 
>> const unsigned int iov_cnt,
>>               offset -= iov[i].iov_len;
>>           }
>>       }
>> -    assert(offset == 0);
>>       return done;
>>   }
>> @@ -74,7 +72,6 @@ size_t iov_memset(const struct iovec *iov, const 
>> unsigned int iov_cnt,
>>               offset -= iov[i].iov_len;
>>           }
>>       }
>> -    assert(offset == 0);
>>       return done;
>>   }
>> @@ -266,7 +263,6 @@ unsigned iov_copy(struct iovec *dst_iov, unsigned 
>> int dst_iov_cnt,
>>           bytes -= len;
>>           offset = 0;
>>       }
>> -    assert(offset == 0);
>>       return j;
>>   }
>> @@ -337,7 +333,6 @@ size_t qemu_iovec_concat_iov(QEMUIOVector *dst,
>>               soffset -= src_iov[i].iov_len;
>>           }
>>       }
>> -    assert(soffset == 0); /* offset beyond end of src */
>>       return done;
>>   }
>>

Re: [PATCH 1/2] util/iov: Do not assert offset is in iov

[Jason Wang]

On Sat, Jan 11, 2025 at 1:11 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Jason, can you pull this series?

Queued.

Thanks

>
> Regards,
> Akihiko Odaki
>
> On 2024/05/08 23:51, Philippe Mathieu-Daudé wrote:
> > ping?
> >
> > On 28/4/24 13:11, Akihiko Odaki wrote:
> >> iov_from_buf(), iov_to_buf(), iov_memset(), and iov_copy() asserts
> >> that the given offset fits in the iov while tolerating the specified
> >> number of bytes to operate with to be greater than the size of iov.
> >> This is inconsistent so remove the assertions.
> >>
> >> Asserting the offset fits in the iov makes sense if it is expected that
> >> there are other operations that process the content before the offset
> >> and the content is processed in order. Under this expectation, the
> >> offset should point to the end of bytes that are previously processed
> >> and fit in the iov. However, this expectation depends on the details of
> >> the caller, and did not hold true at least one case and required code to
> >> check iov_size(), which is added with commit 83ddb3dbba2e
> >> ("hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()").
> >>
> >> Adding such a check is inefficient and error-prone. These functions
> >> already tolerate the specified number of bytes to operate with to be
> >> greater than the size of iov to avoid such checks so remove the
> >> assertions to tolerate invalid offset as well. They return the number of
> >> bytes they operated with so their callers can still check the returned
> >> value to ensure there are sufficient space at the given offset.
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >>   include/qemu/iov.h | 5 +++--
> >>   util/iov.c         | 5 -----
> >>   2 files changed, 3 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> >> index 63a1c01965d1..33548058d2ee 100644
> >> --- a/include/qemu/iov.h
> >> +++ b/include/qemu/iov.h
> >> @@ -30,7 +30,7 @@ size_t iov_size(const struct iovec *iov, const
> >> unsigned int iov_cnt);
> >>    * only part of data will be copied, up to the end of the iovec.
> >>    * Number of bytes actually copied will be returned, which is
> >>    *  min(bytes, iov_size(iov)-offset)
> >> - * `Offset' must point to the inside of iovec.
> >> + * Returns 0 when `offset' points to the outside of iovec.
> >>    */
> >>   size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
> >>                            size_t offset, const void *buf, size_t bytes);
> >> @@ -66,11 +66,12 @@ iov_to_buf(const struct iovec *iov, const unsigned
> >> int iov_cnt,
> >>   /**
> >>    * Set data bytes pointed out by iovec `iov' of size `iov_cnt'
> >> elements,
> >>    * starting at byte offset `start', to value `fillc', repeating it
> >> - * `bytes' number of times.  `Offset' must point to the inside of iovec.
> >> + * `bytes' number of times.
> >>    * If `bytes' is large enough, only last bytes portion of iovec,
> >>    * up to the end of it, will be filled with the specified value.
> >>    * Function return actual number of bytes processed, which is
> >>    * min(size, iov_size(iov) - offset).
> >> + * Returns 0 when `offset' points to the outside of iovec.
> >>    */
> >>   size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
> >>                     size_t offset, int fillc, size_t bytes);
> >> diff --git a/util/iov.c b/util/iov.c
> >> index 7e73948f5e3d..a523b406b7f8 100644
> >> --- a/util/iov.c
> >> +++ b/util/iov.c
> >> @@ -36,7 +36,6 @@ size_t iov_from_buf_full(const struct iovec *iov,
> >> unsigned int iov_cnt,
> >>               offset -= iov[i].iov_len;
> >>           }
> >>       }
> >> -    assert(offset == 0);
> >>       return done;
> >>   }
> >> @@ -55,7 +54,6 @@ size_t iov_to_buf_full(const struct iovec *iov,
> >> const unsigned int iov_cnt,
> >>               offset -= iov[i].iov_len;
> >>           }
> >>       }
> >> -    assert(offset == 0);
> >>       return done;
> >>   }
> >> @@ -74,7 +72,6 @@ size_t iov_memset(const struct iovec *iov, const
> >> unsigned int iov_cnt,
> >>               offset -= iov[i].iov_len;
> >>           }
> >>       }
> >> -    assert(offset == 0);
> >>       return done;
> >>   }
> >> @@ -266,7 +263,6 @@ unsigned iov_copy(struct iovec *dst_iov, unsigned
> >> int dst_iov_cnt,
> >>           bytes -= len;
> >>           offset = 0;
> >>       }
> >> -    assert(offset == 0);
> >>       return j;
> >>   }
> >> @@ -337,7 +333,6 @@ size_t qemu_iovec_concat_iov(QEMUIOVector *dst,
> >>               soffset -= src_iov[i].iov_len;
> >>           }
> >>       }
> >> -    assert(soffset == 0); /* offset beyond end of src */
> >>       return done;
> >>   }
> >>
>

[PATCH 2/2] Revert "hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()"

[Akihiko Odaki]

This reverts commit 83ddb3dbba2ee0f1767442ae6ee665058aeb1093.

The added check is no longer necessary due to a change of
iov_from_buf().

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/net/net_tx_pkt.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
index b7b1de816dc5..2134a18c4c90 100644
--- a/hw/net/net_tx_pkt.c
+++ b/hw/net/net_tx_pkt.c
@@ -141,10 +141,6 @@ bool net_tx_pkt_update_sctp_checksum(struct NetTxPkt *pkt)
     uint32_t csum = 0;
     struct iovec *pl_start_frag = pkt->vec + NET_TX_PKT_PL_START_FRAG;
 
-    if (iov_size(pl_start_frag, pkt->payload_frags) < 8 + sizeof(csum)) {
-        return false;
-    }
-
     if (iov_from_buf(pl_start_frag, pkt->payload_frags, 8, &csum, sizeof(csum)) < sizeof(csum)) {
         return false;
     }

-- 
2.44.0

Re: [PATCH 2/2] Revert "hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()"

[Philippe Mathieu-Daudé]

On 28/4/24 13:11, Akihiko Odaki wrote:
> This reverts commit 83ddb3dbba2ee0f1767442ae6ee665058aeb1093.
> 
> The added check is no longer necessary due to a change of
> iov_from_buf().
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/net/net_tx_pkt.c | 4 ----
>   1 file changed, 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>