Hi Thomas, On Thu, Aug 31, 2023 at 9:40 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: > Am 24.08.23 um 17:08 schrieb Geert Uytterhoeven: > > drm_mode_create_dumb() calculates the number of characters per pixel > > from the number of bits per pixel by rounding up, which is not correct > > as the actual value of cpp may be non-integer. While we do not need to > > care here about complex formats like YUV, bpp < 8 is a valid use case. > > > > - The overflow check for the buffer width is not correct if bpp < 8. > > However, it doesn't hurt, as widths larger than U32_MAX / 8 should > > not happen for real anyway. Add a comment to clarify. > > - Calculating the stride from the number of characters per pixel is > > not correct. Fix this by calculating it from the number of bits per > > pixel instead. > > > > Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> > > Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> > > Tested-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> > > --- > > v2: > > - Add Reviewed-by, Tested-by. > > --- > > drivers/gpu/drm/drm_dumb_buffers.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c > > index 70032bba1c97e787..21a04c32a5e3d785 100644 > > --- a/drivers/gpu/drm/drm_dumb_buffers.c > > +++ b/drivers/gpu/drm/drm_dumb_buffers.c > > @@ -71,10 +71,11 @@ int drm_mode_create_dumb(struct drm_device *dev, > > /* overflow checks for 32bit size calculations */ > > if (args->bpp > U32_MAX - 8) > > return -EINVAL; > > + /* Incorrect (especially if bpp < 8), but doesn't hurt much */ > > cpp = DIV_ROUND_UP(args->bpp, 8); > > if (cpp > U32_MAX / args->width) > > return -EINVAL; > > At first, I was confused by this. Me too ;-) > So I'd really prefer to outright > delete this code. As you say, it's incorrect and doesn't add anything. > The concept of cpp is somewhat wrong anyway. > > > - stride = cpp * args->width; > > + stride = DIV_ROUND_UP(args->bpp * args->width, 8); > > Do we need an overflow check for (bpp * width < U32_MAX) ? I think I thought it would be covered by the above, but on more thought, that is indeed not true. So perhaps drop the cpp code above, and test bpp instead: - cpp = DIV_ROUND_UP(args->bpp, 8); - if (cpp > U32_MAX / args->width) + if (args->bpp > U32_MAX / args->width) return -EINVAL; That would preclude a very wide buffer with height 1, though. BTW, in v1 I also had this question: Why is drm_mode_create_dumb.size __u64? The test for "args->height > U32_MAX / stride" rejects all sizes not fitting in __u32 anyway. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds