BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH bpf] xsk: fix for xp_aligned_validate_desc() when len == chunk_size
@ 2021-04-27 12:19 Xuan Zhuo
  2021-04-28  8:00 ` Magnus Karlsson
  0 siblings, 1 reply; 3+ messages in thread
From: Xuan Zhuo @ 2021-04-27 12:19 UTC (permalink / raw)
  To: bpf
  Cc: Björn Töpel, Magnus Karlsson, Jonathan Lemon,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	KP Singh, Maxim Mikityanskiy, netdev

When desc->len is equal to chunk_size, it is legal. But
xp_aligned_validate_desc() got "chunk_end" by desc->addr + desc->len
pointing to the next chunk during the check, which caused the check to
fail.

Fixes: 35fcde7f8deb ("xsk: support for Tx")
Fixes: bbff2f321a86 ("xsk: new descriptor addressing scheme")
Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")
Fixes: 26062b185eee ("xsk: Explicitly inline functions and move definitions")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 net/xdp/xsk_queue.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index 2823b7c3302d..40f359bf2044 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -128,13 +128,12 @@ static inline bool xskq_cons_read_addr_unchecked(struct xsk_queue *q, u64 *addr)
 static inline bool xp_aligned_validate_desc(struct xsk_buff_pool *pool,
 					    struct xdp_desc *desc)
 {
-	u64 chunk, chunk_end;
+	u64 chunk;
 
-	chunk = xp_aligned_extract_addr(pool, desc->addr);
-	chunk_end = xp_aligned_extract_addr(pool, desc->addr + desc->len);
-	if (chunk != chunk_end)
+	if (desc->len > pool->chunk_size)
 		return false;
 
+	chunk = xp_aligned_extract_addr(pool, desc->addr);
 	if (chunk >= pool->addrs_cnt)
 		return false;
 
-- 
2.31.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf] xsk: fix for xp_aligned_validate_desc() when len == chunk_size
  2021-04-27 12:19 [PATCH bpf] xsk: fix for xp_aligned_validate_desc() when len == chunk_size Xuan Zhuo
@ 2021-04-28  8:00 ` Magnus Karlsson
  2021-04-28  9:09   ` Daniel Borkmann
  0 siblings, 1 reply; 3+ messages in thread
From: Magnus Karlsson @ 2021-04-28  8:00 UTC (permalink / raw)
  To: Xuan Zhuo
  Cc: bpf, Björn Töpel, Magnus Karlsson, Jonathan Lemon,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	KP Singh, Maxim Mikityanskiy, Network Development

On Tue, Apr 27, 2021 at 2:19 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> When desc->len is equal to chunk_size, it is legal. But
> xp_aligned_validate_desc() got "chunk_end" by desc->addr + desc->len
> pointing to the next chunk during the check, which caused the check to
> fail.

Thanks Xuan for the fix. Off-by-one error. A classic unfortunately.

Think your fix also makes it easier to understand the code too, so good.

> Fixes: 35fcde7f8deb ("xsk: support for Tx")
> Fixes: bbff2f321a86 ("xsk: new descriptor addressing scheme")

Just did some quick research and it seems the bug was introduced in
the bbff2f321a86 commit above, not the first one 35fcde7f8deb. Or am I
mistaken?

> Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")
> Fixes: 26062b185eee ("xsk: Explicitly inline functions and move definitions")

And in these two, the code was moved around first to a new function in
2b43470add8c, then this function was moved to a new file in
26062b185eee. I believe documenting this in your commit message would
make life simpler for the nice people backporting this fix. Or is this
implicit in the multiple Fixes tags? Could someone with more
experience in these things comment please.

Thank you: Magnus

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  net/xdp/xsk_queue.h | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
> index 2823b7c3302d..40f359bf2044 100644
> --- a/net/xdp/xsk_queue.h
> +++ b/net/xdp/xsk_queue.h
> @@ -128,13 +128,12 @@ static inline bool xskq_cons_read_addr_unchecked(struct xsk_queue *q, u64 *addr)
>  static inline bool xp_aligned_validate_desc(struct xsk_buff_pool *pool,
>                                             struct xdp_desc *desc)
>  {
> -       u64 chunk, chunk_end;
> +       u64 chunk;
>
> -       chunk = xp_aligned_extract_addr(pool, desc->addr);
> -       chunk_end = xp_aligned_extract_addr(pool, desc->addr + desc->len);
> -       if (chunk != chunk_end)
> +       if (desc->len > pool->chunk_size)
>                 return false;
>
> +       chunk = xp_aligned_extract_addr(pool, desc->addr);
>         if (chunk >= pool->addrs_cnt)
>                 return false;
>
> --
> 2.31.0
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf] xsk: fix for xp_aligned_validate_desc() when len == chunk_size
  2021-04-28  8:00 ` Magnus Karlsson
@ 2021-04-28  9:09   ` Daniel Borkmann
  0 siblings, 0 replies; 3+ messages in thread
From: Daniel Borkmann @ 2021-04-28  9:09 UTC (permalink / raw)
  To: Magnus Karlsson, Xuan Zhuo
  Cc: bpf, Björn Töpel, Magnus Karlsson, Jonathan Lemon,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Jesper Dangaard Brouer, John Fastabend, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, KP Singh,
	Maxim Mikityanskiy, Network Development

On 4/28/21 10:00 AM, Magnus Karlsson wrote:
> On Tue, Apr 27, 2021 at 2:19 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>>
>> When desc->len is equal to chunk_size, it is legal. But
>> xp_aligned_validate_desc() got "chunk_end" by desc->addr + desc->len
>> pointing to the next chunk during the check, which caused the check to
>> fail.
> 
> Thanks Xuan for the fix. Off-by-one error. A classic unfortunately.
> 
> Think your fix also makes it easier to understand the code too, so good.
> 
>> Fixes: 35fcde7f8deb ("xsk: support for Tx")
>> Fixes: bbff2f321a86 ("xsk: new descriptor addressing scheme")
> 
> Just did some quick research and it seems the bug was introduced in
> the bbff2f321a86 commit above, not the first one 35fcde7f8deb. Or am I
> mistaken?
> 
>> Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")
>> Fixes: 26062b185eee ("xsk: Explicitly inline functions and move definitions")
> 
> And in these two, the code was moved around first to a new function in
> 2b43470add8c, then this function was moved to a new file in
> 26062b185eee. I believe documenting this in your commit message would
> make life simpler for the nice people backporting this fix. Or is this
> implicit in the multiple Fixes tags? Could someone with more
> experience in these things comment please.

Fully agree with Magnus, providing more context in the commit message is
always appreciated. Xuan, please extend and resubmit. Thanks!

> Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> 
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>   net/xdp/xsk_queue.h | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
>> index 2823b7c3302d..40f359bf2044 100644
>> --- a/net/xdp/xsk_queue.h
>> +++ b/net/xdp/xsk_queue.h
>> @@ -128,13 +128,12 @@ static inline bool xskq_cons_read_addr_unchecked(struct xsk_queue *q, u64 *addr)
>>   static inline bool xp_aligned_validate_desc(struct xsk_buff_pool *pool,
>>                                              struct xdp_desc *desc)
>>   {
>> -       u64 chunk, chunk_end;
>> +       u64 chunk;
>>
>> -       chunk = xp_aligned_extract_addr(pool, desc->addr);
>> -       chunk_end = xp_aligned_extract_addr(pool, desc->addr + desc->len);
>> -       if (chunk != chunk_end)
>> +       if (desc->len > pool->chunk_size)
>>                  return false;
>>
>> +       chunk = xp_aligned_extract_addr(pool, desc->addr);
>>          if (chunk >= pool->addrs_cnt)
>>                  return false;
>>
>> --
>> 2.31.0
>>


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-27 12:19 [PATCH bpf] xsk: fix for xp_aligned_validate_desc() when len == chunk_size Xuan Zhuo
2021-04-28  8:00 ` Magnus Karlsson
2021-04-28  9:09   ` Daniel Borkmann

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git