Danilo Krummrich
2024-Apr-08 13:23 UTC
[PATCH] drm: nv04: Add check to avoid out of bounds access
On 4/5/24 22:05, Lyude Paul wrote:> On Fri, 2024-04-05 at 17:53 +0200, Danilo Krummrich wrote: >> On 3/31/24 08:45, Mikhail Kobuk wrote: >>> Output Resource (dcb->or) value is not guaranteed to be non-zero >>> (i.e. >>> in drivers/gpu/drm/nouveau/nouveau_bios.c, in >>> 'fabricate_dcb_encoder_table()' >>> 'dcb->or' is assigned value '0' in call to >>> 'fabricate_dcb_output()'). >> >> I don't really know much about the semantics of this code. >> >> Looking at fabricate_dcb_output() though I wonder if the intention >> was to assign >> BIT(or) to entry->or. >> >> @Lyude, can you help here? > > This code is definitely a bit before my time as well - but I think > you're completely correct. Especially considering this bit I found in > nouveau_bios.h:Thanks for confirming. @Mikhail, I think we should rather fix this assignment then. - Danilo> > enum nouveau_or { > DCB_OUTPUT_A = (1 << 0), > DCB_OUTPUT_B = (1 << 1), > DCB_OUTPUT_C = (1 << 2) > }; > > >> >> Otherwise, for parsing the DCB entries, it seems that the bound >> checks are >> happening in olddcb_outp_foreach() [1]. >> >> [1] >> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/nouveau/nouveau_bios.c#L1331 >> >>> >>> Add check to validate 'dcb->or' before it's used. >>> >>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> >>> Fixes: 2e5702aff395 ("drm/nouveau: fabricate DCB encoder table for >>> iMac G4") >>> Signed-off-by: Mikhail Kobuk <m.kobuk at ispras.ru> >>> --- >>> ? drivers/gpu/drm/nouveau/dispnv04/dac.c | 4 ++-- >>> ? 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/nouveau/dispnv04/dac.c >>> b/drivers/gpu/drm/nouveau/dispnv04/dac.c >>> index d6b8e0cce2ac..0c8d4fc95ff3 100644 >>> --- a/drivers/gpu/drm/nouveau/dispnv04/dac.c >>> +++ b/drivers/gpu/drm/nouveau/dispnv04/dac.c >>> @@ -428,7 +428,7 @@ void nv04_dac_update_dacclk(struct drm_encoder >>> *encoder, bool enable) >>> ?? struct drm_device *dev = encoder->dev; >>> ?? struct dcb_output *dcb = nouveau_encoder(encoder)->dcb; >>> >>> - if (nv_gf4_disp_arch(dev)) { >>> + if (nv_gf4_disp_arch(dev) && ffs(dcb->or)) { >>> ?? uint32_t *dac_users = &nv04_display(dev)- >>>> dac_users[ffs(dcb->or) - 1]; >>> ?? int dacclk_off = NV_PRAMDAC_DACCLK + >>> nv04_dac_output_offset(encoder); >>> ?? uint32_t dacclk = NVReadRAMDAC(dev, 0, >>> dacclk_off); >>> @@ -453,7 +453,7 @@ bool nv04_dac_in_use(struct drm_encoder >>> *encoder) >>> ?? struct drm_device *dev = encoder->dev; >>> ?? struct dcb_output *dcb = nouveau_encoder(encoder)->dcb; >>> >>> - return nv_gf4_disp_arch(encoder->dev) && >>> + return nv_gf4_disp_arch(encoder->dev) && ffs(dcb->or) && >>> ?? (nv04_display(dev)->dac_users[ffs(dcb->or) - 1] & >>> ~(1 << dcb->index)); >>> ? } >>> >> >
Mikhail Kobuk
2024-Apr-10 15:39 UTC
[PATCH] drm: nv04: Add check to avoid out of bounds access
On 08/04/2024 16:23, Danilo Krummrich wrote:> On 4/5/24 22:05, Lyude Paul wrote: >> On Fri, 2024-04-05 at 17:53 +0200, Danilo Krummrich wrote: >>> On 3/31/24 08:45, Mikhail Kobuk wrote: >>>> Output Resource (dcb->or) value is not guaranteed to be non-zero >>>> (i.e. >>>> in drivers/gpu/drm/nouveau/nouveau_bios.c, in >>>> 'fabricate_dcb_encoder_table()' >>>> 'dcb->or' is assigned value '0' in call to >>>> 'fabricate_dcb_output()'). >>> >>> I don't really know much about the semantics of this code. >>> >>> Looking at fabricate_dcb_output() though I wonder if the intention >>> was to assign >>> BIT(or) to entry->or. >>> >>> @Lyude, can you help here? >> >> This code is definitely a bit before my time as well - but I think >> you're completely correct. Especially considering this bit I found in >> nouveau_bios.h: > > Thanks for confirming. > > @Mikhail, I think we should rather fix this assignment then.Thank you all for a thorough look!> > - Danilo > >> >> enum nouveau_or { >> DCB_OUTPUT_A = (1 << 0), >> DCB_OUTPUT_B = (1 << 1), >> DCB_OUTPUT_C = (1 << 2) >> }; >> >>Considering this code bit, and the fact that fabricate_dcb_output() is called in drivers/gpu/drm/nouveau/nouveau_bios.c only, there's option to adjust function calls instead of adding BIT(or), i.e.: fabricate_dcb_output(dcb, DCB_OUTPUT_TMDS, 0, all_heads, DCB_OUTPUT_B); instead of current: fabricate_dcb_output(dcb, DCB_OUTPUT_TMDS, 0, all_heads, 1); and etc. Should I make a new patch with adjusted calls or stick with BIT(or)?>>> >>> Otherwise, for parsing the DCB entries, it seems that the bound >>> checks are >>> happening in olddcb_outp_foreach() [1]. >>> >>> [1] >>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/nouveau/nouveau_bios.c#L1331 >>> >>>> >>>> Add check to validate 'dcb->or' before it's used. >>>> >>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>> >>>> Fixes: 2e5702aff395 ("drm/nouveau: fabricate DCB encoder table for >>>> iMac G4") >>>> Signed-off-by: Mikhail Kobuk <m.kobuk at ispras.ru> >>>> --- >>>> ? drivers/gpu/drm/nouveau/dispnv04/dac.c | 4 ++-- >>>> ? 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/nouveau/dispnv04/dac.c >>>> b/drivers/gpu/drm/nouveau/dispnv04/dac.c >>>> index d6b8e0cce2ac..0c8d4fc95ff3 100644 >>>> --- a/drivers/gpu/drm/nouveau/dispnv04/dac.c >>>> +++ b/drivers/gpu/drm/nouveau/dispnv04/dac.c >>>> @@ -428,7 +428,7 @@ void nv04_dac_update_dacclk(struct drm_encoder >>>> *encoder, bool enable) >>>> ?? struct drm_device *dev = encoder->dev; >>>> ?? struct dcb_output *dcb = nouveau_encoder(encoder)->dcb; >>>> >>>> - if (nv_gf4_disp_arch(dev)) { >>>> + if (nv_gf4_disp_arch(dev) && ffs(dcb->or)) { >>>> ?? uint32_t *dac_users = &nv04_display(dev)- >>>>> dac_users[ffs(dcb->or) - 1]; >>>> ?? int dacclk_off = NV_PRAMDAC_DACCLK + >>>> nv04_dac_output_offset(encoder); >>>> ?? uint32_t dacclk = NVReadRAMDAC(dev, 0, >>>> dacclk_off); >>>> @@ -453,7 +453,7 @@ bool nv04_dac_in_use(struct drm_encoder >>>> *encoder) >>>> ?? struct drm_device *dev = encoder->dev; >>>> ?? struct dcb_output *dcb = nouveau_encoder(encoder)->dcb; >>>> >>>> - return nv_gf4_disp_arch(encoder->dev) && >>>> + return nv_gf4_disp_arch(encoder->dev) && ffs(dcb->or) && >>>> ?? (nv04_display(dev)->dac_users[ffs(dcb->or) - 1] & >>>> ~(1 << dcb->index)); >>>> ? } >>>> >>> >>
Danilo Krummrich
2024-Apr-10 16:24 UTC
[PATCH] drm: nv04: Add check to avoid out of bounds access
On 4/10/24 17:39, Mikhail Kobuk wrote:> On 08/04/2024 16:23, Danilo Krummrich wrote: >> On 4/5/24 22:05, Lyude Paul wrote: >>> On Fri, 2024-04-05 at 17:53 +0200, Danilo Krummrich wrote: >>>> On 3/31/24 08:45, Mikhail Kobuk wrote: >>>>> Output Resource (dcb->or) value is not guaranteed to be non-zero >>>>> (i.e. >>>>> in drivers/gpu/drm/nouveau/nouveau_bios.c, in >>>>> 'fabricate_dcb_encoder_table()' >>>>> 'dcb->or' is assigned value '0' in call to >>>>> 'fabricate_dcb_output()'). >>>> >>>> I don't really know much about the semantics of this code. >>>> >>>> Looking at fabricate_dcb_output() though I wonder if the intention >>>> was to assign >>>> BIT(or) to entry->or. >>>> >>>> @Lyude, can you help here? >>> >>> This code is definitely a bit before my time as well - but I think >>> you're completely correct. Especially considering this bit I found in >>> nouveau_bios.h: >> >> Thanks for confirming. >> >> @Mikhail, I think we should rather fix this assignment then. > > Thank you all for a thorough look! > >> >> - Danilo >> >>> >>> enum nouveau_or { >>> ????DCB_OUTPUT_A = (1 << 0), >>> ????DCB_OUTPUT_B = (1 << 1), >>> ????DCB_OUTPUT_C = (1 << 2) >>> }; >>> >>> > > Considering this code bit, and the fact that fabricate_dcb_output() is called in drivers/gpu/drm/nouveau/nouveau_bios.c only, there's option to adjust function calls instead of adding BIT(or), i.e.: > > fabricate_dcb_output(dcb, DCB_OUTPUT_TMDS, 0, all_heads, DCB_OUTPUT_B); > > instead of current: > > fabricate_dcb_output(dcb, DCB_OUTPUT_TMDS, 0, all_heads, 1); > > and etc. > > Should I make a new patch with adjusted calls or stick with BIT(or)?Please send a new patch adjusting the calls using enum nouveau_or, that seems to be cleaner. - Danilo> >>>> >>>> Otherwise, for parsing the DCB entries, it seems that the bound >>>> checks are >>>> happening in olddcb_outp_foreach() [1]. >>>> >>>> [1] >>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/nouveau/nouveau_bios.c#L1331 >>>> >>>>> >>>>> Add check to validate 'dcb->or' before it's used. >>>>> >>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>>> >>>>> Fixes: 2e5702aff395 ("drm/nouveau: fabricate DCB encoder table for >>>>> iMac G4") >>>>> Signed-off-by: Mikhail Kobuk <m.kobuk at ispras.ru> >>>>> --- >>>>> ?? drivers/gpu/drm/nouveau/dispnv04/dac.c | 4 ++-- >>>>> ?? 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/nouveau/dispnv04/dac.c >>>>> b/drivers/gpu/drm/nouveau/dispnv04/dac.c >>>>> index d6b8e0cce2ac..0c8d4fc95ff3 100644 >>>>> --- a/drivers/gpu/drm/nouveau/dispnv04/dac.c >>>>> +++ b/drivers/gpu/drm/nouveau/dispnv04/dac.c >>>>> @@ -428,7 +428,7 @@ void nv04_dac_update_dacclk(struct drm_encoder >>>>> *encoder, bool enable) >>>>> ?????? struct drm_device *dev = encoder->dev; >>>>> ?????? struct dcb_output *dcb = nouveau_encoder(encoder)->dcb; >>>>> -??? if (nv_gf4_disp_arch(dev)) { >>>>> +??? if (nv_gf4_disp_arch(dev) && ffs(dcb->or)) { >>>>> ?????????? uint32_t *dac_users = &nv04_display(dev)- >>>>>> dac_users[ffs(dcb->or) - 1]; >>>>> ?????????? int dacclk_off = NV_PRAMDAC_DACCLK + >>>>> nv04_dac_output_offset(encoder); >>>>> ?????????? uint32_t dacclk = NVReadRAMDAC(dev, 0, >>>>> dacclk_off); >>>>> @@ -453,7 +453,7 @@ bool nv04_dac_in_use(struct drm_encoder >>>>> *encoder) >>>>> ?????? struct drm_device *dev = encoder->dev; >>>>> ?????? struct dcb_output *dcb = nouveau_encoder(encoder)->dcb; >>>>> -??? return nv_gf4_disp_arch(encoder->dev) && >>>>> +??? return nv_gf4_disp_arch(encoder->dev) && ffs(dcb->or) && >>>>> ?????????? (nv04_display(dev)->dac_users[ffs(dcb->or) - 1] & >>>>> ~(1 << dcb->index)); >>>>> ?? } >>>> >>> >