> -----Original Message-----
> From: Luca Coelho <luca@xxxxxxxxx>
> Sent: Tuesday, October 31, 2023 1:14 PM
> To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Subject: Re: [PATCH v4] drm/i915/dsb: DSB code refactoring
>
> On Fri, 2023-10-27 at 17:27 +0530, Animesh Manna wrote:
> > Refactor DSB implementation to be compatible with Xe driver.
> >
> > v1: RFC version.
> > v2: Make intel_dsb structure opaque from external usage. [Jani]
> > v3: Rebased on latest.
> > v4:
> > - Add boundary check in dsb_buffer_memset(). [Luca]
> > - Use size_t instead of u32. [Luca]
> >
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> > ---
>
> [...]
> > +void intel_dsb_buffer_memset(struct intel_dsb_buffer *dsb_buf, u32
> > +idx, u32 val, size_t size) {
> > + if ((idx > dsb_buf->buf_size / 4) || (size > dsb_buf->buf_size - idx
> > +* 4))
>
> You actually don't need the first expression. This expression should
> enough:
>
> dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf->cmd_buf)
Here size is in bytes, but idx is index of 32 bytes array. So, the above expression would be,
dsb_buf->buf_size <= (idx * sizeof(*dsb_buf->cmd_buf) + size)
The same is done with 2nd expression but agree to use sizeof() instead of magic number 4.
The first expression is added if idx is big number and due to overflow the above check can pass which is not correct. Please let me know your thoughts, if you are not ok will drop maybe.
>
> > + return;
>
> Blindly returning here doesn't solve the problem, it just hides it. I think the
> best would be to use WARN_ON() instead of if.
>
> So:
> WARN_ON(dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf-
> >cmd_buf));
I will add the WARN_ON().
Regards,
Animesh
>
> > +
> > + memset(&dsb_buf->cmd_buf[idx], val, size); }
> [...]
>
> --
> Cheers,
> Luca.
