| Message ID | 16d488639e99f43ca3977ee7b8f76fc26c34aa86.1692888745.git.geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1 | expand |
Hi 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@linux-m68k.org> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Tested-by: Javier Martinez Canillas <javierm@redhat.com> > --- > 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. 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) ? Best regards Thomas > if (args->height > U32_MAX / stride) > return -EINVAL; >
Hi Thomas, On Thu, Aug 31, 2023 at 9:40 AM Thomas Zimmermann <tzimmermann@suse.de> 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@linux-m68k.org> > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > Tested-by: Javier Martinez Canillas <javierm@redhat.com> > > --- > > 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@linux-m68k.org 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
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; - stride = cpp * args->width; + stride = DIV_ROUND_UP(args->bpp * args->width, 8); if (args->height > U32_MAX / stride) return -EINVAL;
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@linux-m68k.org> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Tested-by: Javier Martinez Canillas <javierm@redhat.com> --- v2: - Add Reviewed-by, Tested-by. --- drivers/gpu/drm/drm_dumb_buffers.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)