Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Unvalidated device tree properties allow out-of-bounds array access and memory corruption. - [High] Unconditional hardware state reset clobbers interrupt states and wait bases for unowned syncpoints. --
commit bdd04d3945b8f8760d8be70ce0ab1595927b08f4 Author: Mikko Perttunen <[email protected]> gpu: host1x: Allow limiting usable channel and syncpoint ranges Allow specifying usable channel, and allocatable syncpoint ranges through the device tree. This is necessary when Host1x resources are shared between multiple operating systems or independent hardware units and Linux has limited access to them. > diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c > index ca5d0f51cf7da..38fcdda8388ad 100644 > --- a/drivers/gpu/host1x/channel.c > +++ b/drivers/gpu/host1x/channel.c > @@ -121,13 +121,13 @@ EXPORT_SYMBOL(host1x_channel_put); > static struct host1x_channel *acquire_unused_channel(struct host1x *host) > { > struct host1x_channel_list *chlist = &host->channel_list; > - unsigned int max_channels = host->info->nb_channels; > unsigned int index; > > mutex_lock(&chlist->lock); > > - index = find_first_zero_bit(chlist->allocated_channels, max_channels); > - if (index >= max_channels) { > + index = find_next_zero_bit(chlist->allocated_channels, > + host->channel_end, host->channel_base); [Severity: High] Does this code allow out-of-bounds memory accesses if the device tree values exceed the hardware limits? Since chlist->channels and chlist->allocated_channels are allocated based on host->info->nb_channels, could a large value in the nvidia,channels device tree property cause an out-of-bounds access here when host->channel_end is passed to find_next_zero_bit()? Should host->channel_end be validated in host1x_get_assigned_resources()? [ ... ] > diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c > index 545fa2e3f180c..7103f018cb1bf 100644 > --- a/drivers/gpu/host1x/dev.c > +++ b/drivers/gpu/host1x/dev.c > @@ -331,6 +331,39 @@ static void host1x_setup_virtualization_tables(struct > host1x *host) > } > } > > +static int host1x_get_assigned_resources(struct host1x *host) > +{ > + struct device_node *np = host->dev->of_node; > + u32 vals[2]; > + int err; > + > + err = of_property_read_u32_array(np, "nvidia,channels", vals, 2); > + if (err == 0) { > + host->channel_base = vals[0]; > + host->channel_end = vals[0] + vals[1]; > + } else if (err == -EINVAL) { > + host->channel_base = 0; > + host->channel_end = host->info->nb_channels; > + } else { > + dev_err(host->dev, "invalid nvidia,channels property: %d\n", > err); > + return err; > + } > + > + err = of_property_read_u32_array(np, "nvidia,syncpoints", vals, 2); > + if (err == 0) { > + host->syncpt_base = vals[0]; > + host->syncpt_end = vals[0] + vals[1]; [Severity: High] Similarly, could large values for nvidia,syncpoints cause out-of-bounds reads and writes? Arrays like host->syncpt are allocated based on host->info->nb_pts. If host->syncpt_end exceeds this limit without validation, functions like host1x_syncpt_alloc() that iterate up to host->syncpt_end might access memory out of bounds. [ ... ] > diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c > index acc7d82e0585e..fe27a386cc0c8 100644 > --- a/drivers/gpu/host1x/syncpt.c > +++ b/drivers/gpu/host1x/syncpt.c [ ... ] > @@ -138,7 +138,7 @@ void host1x_syncpt_restore(struct host1x *host) > struct host1x_syncpt *sp_base = host->syncpt; > unsigned int i; > > - for (i = 0; i < host1x_syncpt_nb_pts(host); i++) { > + for (i = host->syncpt_base; i < host->syncpt_end; i++) { > /* > * Unassign syncpt from channels for purposes of Tegra186 > * syncpoint protection. This prevents any channel from [Severity: High] Does this clobber the hardware state for unowned syncpoints? Right after this loop, the code in host1x_syncpt_restore() unconditionally restores wait bases for syncpoints 0 to nb_bases-1: for (i = 0; i < host1x_syncpt_nb_bases(host); i++) host1x_hw_syncpt_restore_wait_base(host, sp_base + i); Similarly, host1x_intr_disable_all_syncpt_intrs() unconditionally disables and clears threshold interrupts for all syncpoints across the entire hardware. Since this patch restricts Linux to a specific range of syncpoints to allow sharing the Host1x device, will overwriting the wait bases and interrupt states unconditionally break synchronization for other operating systems or hardware units sharing the device? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
