| Message ID | 15311f1c044c3ff26624e2a980b0c477b1cf33b2.1495498184.git.digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reloc shifts are implemented (see assignment of u32 reloc_addr in do_relocs), and they are required for VIC job submissions. On 23.05.2017 03:14, Dmitry Osipenko wrote: > Incorrectly shifted relocation address will cause a lower memory corruption > and likely a hang on a write or a read of an arbitrary data in case of IOMMU > absent. As of now there is no use for the address shifting (at least on > Tegra20) and adding a proper shifting / sizes validation is much more work. > Let's forbid it in the firewall till a proper validation is implemented. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/gpu/host1x/job.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c > index 190353944d23..1a1568e64ba8 100644 > --- a/drivers/gpu/host1x/job.c > +++ b/drivers/gpu/host1x/job.c > @@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf, > if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset) > return false; > > + /* relocation shift value validation isn't implemented yet */ > + if (reloc->shift) > + return false; > + > return true; > } > >
On 23.05.2017 13:14, Mikko Perttunen wrote: > Reloc shifts are implemented (see assignment of u32 reloc_addr in do_relocs), > and they are required for VIC job submissions. > The *validation* is unimplemented. Since VIC uses the shifts, we may add a validation for it in a way it is done for the registers / class checks, i.e. compare the per address register shift value. I suppose those registers are documented somewhere(TRM), could you please point me to them (TRM page, reg name)? > On 23.05.2017 03:14, Dmitry Osipenko wrote: >> Incorrectly shifted relocation address will cause a lower memory corruption >> and likely a hang on a write or a read of an arbitrary data in case of IOMMU >> absent. As of now there is no use for the address shifting (at least on >> Tegra20) and adding a proper shifting / sizes validation is much more work. >> Let's forbid it in the firewall till a proper validation is implemented. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/gpu/host1x/job.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c >> index 190353944d23..1a1568e64ba8 100644 >> --- a/drivers/gpu/host1x/job.c >> +++ b/drivers/gpu/host1x/job.c >> @@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc, >> struct host1x_bo *cmdbuf, >> if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset) >> return false; >> >> + /* relocation shift value validation isn't implemented yet */ >> + if (reloc->shift) >> + return false; >> + >> return true; >> } >> >>
On 23.05.2017 13:58, Dmitry Osipenko wrote: > On 23.05.2017 13:14, Mikko Perttunen wrote: >> Reloc shifts are implemented (see assignment of u32 reloc_addr in do_relocs), >> and they are required for VIC job submissions. >> > > The *validation* is unimplemented. Since VIC uses the shifts, we may add a > validation for it in a way it is done for the registers / class checks, i.e. > compare the per address register shift value. I suppose those registers are > documented somewhere(TRM), could you please point me to them (TRM page, reg name)? Ah, indeed, sorry. I'm not sure if it's worth implementing validation for e.g. VIC, since it would be quite a bit of work and require many per-chip and per-unit tables or ranges in the kernel. I think it's a better solution to just forbid everything in the firewall for VIC (and eventually other units), since on systems with those units the normal configuration will be IOMMU anyway. Anyway, For VIC, everything happens using two registers (method index and method parameter), so you would need to first detect the method index, save that, then wait for a method parameter write and then check that against the written method. The TRM currently has a list methods, but doesn't list their parameter - at least most ones taking a pointer are called *_OFFSET, but not sure if all. In any case, I don't think it's worth implementing. Thanks, Mikko > >> On 23.05.2017 03:14, Dmitry Osipenko wrote: >>> Incorrectly shifted relocation address will cause a lower memory corruption >>> and likely a hang on a write or a read of an arbitrary data in case of IOMMU >>> absent. As of now there is no use for the address shifting (at least on >>> Tegra20) and adding a proper shifting / sizes validation is much more work. >>> Let's forbid it in the firewall till a proper validation is implemented. >>> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> drivers/gpu/host1x/job.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c >>> index 190353944d23..1a1568e64ba8 100644 >>> --- a/drivers/gpu/host1x/job.c >>> +++ b/drivers/gpu/host1x/job.c >>> @@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc, >>> struct host1x_bo *cmdbuf, >>> if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset) >>> return false; >>> >>> + /* relocation shift value validation isn't implemented yet */ >>> + if (reloc->shift) >>> + return false; >>> + >>> return true; >>> } >>> >>> > >
On 23.05.2017 14:19, Mikko Perttunen wrote: > On 23.05.2017 13:58, Dmitry Osipenko wrote: >> On 23.05.2017 13:14, Mikko Perttunen wrote: >>> Reloc shifts are implemented (see assignment of u32 reloc_addr in do_relocs), >>> and they are required for VIC job submissions. >>> >> >> The *validation* is unimplemented. Since VIC uses the shifts, we may add a >> validation for it in a way it is done for the registers / class checks, i.e. >> compare the per address register shift value. I suppose those registers are >> documented somewhere(TRM), could you please point me to them (TRM page, reg >> name)? > > Ah, indeed, sorry. I'm not sure if it's worth implementing validation for e.g. > VIC, since it would be quite a bit of work and require many per-chip and > per-unit tables or ranges in the kernel. I think it's a better solution to just > forbid everything in the firewall for VIC (and eventually other units), since on > systems with those units the normal configuration will be IOMMU anyway. > > Anyway, For VIC, everything happens using two registers (method index and method > parameter), so you would need to first detect the method index, save that, then > wait for a method parameter write and then check that against the written > method. The TRM currently has a list methods, but doesn't list their parameter - > at least most ones taking a pointer are called *_OFFSET, but not sure if all. In > any case, I don't think it's worth implementing. > Yes, in IOMMU case firewall will be only useful as a debug feature. We may check for the IOMMU presence and bypass the firewall if it's present, or we may bypass the shifts validation only. >> >>> On 23.05.2017 03:14, Dmitry Osipenko wrote: >>>> Incorrectly shifted relocation address will cause a lower memory corruption >>>> and likely a hang on a write or a read of an arbitrary data in case of IOMMU >>>> absent. As of now there is no use for the address shifting (at least on >>>> Tegra20) and adding a proper shifting / sizes validation is much more work. >>>> Let's forbid it in the firewall till a proper validation is implemented. >>>> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> drivers/gpu/host1x/job.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c >>>> index 190353944d23..1a1568e64ba8 100644 >>>> --- a/drivers/gpu/host1x/job.c >>>> +++ b/drivers/gpu/host1x/job.c >>>> @@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc, >>>> struct host1x_bo *cmdbuf, >>>> if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset) >>>> return false; >>>> >>>> + /* relocation shift value validation isn't implemented yet */ >>>> + if (reloc->shift) >>>> + return false; >>>> + >>>> return true; >>>> } >>>> >>>> >> >>
Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com> On 05/23/2017 03:14 AM, Dmitry Osipenko wrote: > Incorrectly shifted relocation address will cause a lower memory corruption > and likely a hang on a write or a read of an arbitrary data in case of IOMMU > absent. As of now there is no use for the address shifting (at least on > Tegra20) and adding a proper shifting / sizes validation is much more work. Perhaps change to "As of now there is no use for the address shifting on Tegra20" if you post another revision. > Let's forbid it in the firewall till a proper validation is implemented. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/gpu/host1x/job.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c > index 190353944d23..1a1568e64ba8 100644 > --- a/drivers/gpu/host1x/job.c > +++ b/drivers/gpu/host1x/job.c > @@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf, > if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset) > return false; > > + /* relocation shift value validation isn't implemented yet */ > + if (reloc->shift) > + return false; > + > return true; > } > >
On 01.06.2017 20:39, Mikko Perttunen wrote: > Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com> > > On 05/23/2017 03:14 AM, Dmitry Osipenko wrote: >> Incorrectly shifted relocation address will cause a lower memory corruption >> and likely a hang on a write or a read of an arbitrary data in case of IOMMU >> absent. As of now there is no use for the address shifting (at least on >> Tegra20) and adding a proper shifting / sizes validation is much more work. > > Perhaps change to "As of now there is no use for the address shifting on > Tegra20" if you post another revision. > I'll post a new revision of the series after getting comments to the all patches, to not churn the ML. Thank you very much for the reviews!
On 01.06.2017 21:37, Dmitry Osipenko wrote: > On 01.06.2017 20:39, Mikko Perttunen wrote: >> Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com> >> >> On 05/23/2017 03:14 AM, Dmitry Osipenko wrote: >>> Incorrectly shifted relocation address will cause a lower memory corruption >>> and likely a hang on a write or a read of an arbitrary data in case of IOMMU >>> absent. As of now there is no use for the address shifting (at least on >>> Tegra20) and adding a proper shifting / sizes validation is much more work. >> >> Perhaps change to "As of now there is no use for the address shifting on >> Tegra20" if you post another revision. >> > I'll post a new revision of the series after getting comments to the all > patches, to not churn the ML. Thank you very much for the reviews! > However, given your previous comments to this patch, I'll probably add a bypass of the shit checking in case of IOMMU presence.
On 06/01/2017 09:44 PM, Dmitry Osipenko wrote: > On 01.06.2017 21:37, Dmitry Osipenko wrote: >> On 01.06.2017 20:39, Mikko Perttunen wrote: >>> Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com> >>> >>> On 05/23/2017 03:14 AM, Dmitry Osipenko wrote: >>>> Incorrectly shifted relocation address will cause a lower memory corruption >>>> and likely a hang on a write or a read of an arbitrary data in case of IOMMU >>>> absent. As of now there is no use for the address shifting (at least on >>>> Tegra20) and adding a proper shifting / sizes validation is much more work. >>> >>> Perhaps change to "As of now there is no use for the address shifting on >>> Tegra20" if you post another revision. >>> >> I'll post a new revision of the series after getting comments to the all >> patches, to not churn the ML. Thank you very much for the reviews! >> > > However, given your previous comments to this patch, I'll probably add a bypass > of the shit checking in case of IOMMU presence. > I don't think that's needed - the firewall will deny pretty much all VIC submissions due to is_addr_reg not being implemented so it cannot reasonably be used on modern Tegras anyway.
On 01.06.2017 21:51, Mikko Perttunen wrote: > On 06/01/2017 09:44 PM, Dmitry Osipenko wrote: >> On 01.06.2017 21:37, Dmitry Osipenko wrote: >>> On 01.06.2017 20:39, Mikko Perttunen wrote: >>>> Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com> >>>> >>>> On 05/23/2017 03:14 AM, Dmitry Osipenko wrote: >>>>> Incorrectly shifted relocation address will cause a lower memory corruption >>>>> and likely a hang on a write or a read of an arbitrary data in case of IOMMU >>>>> absent. As of now there is no use for the address shifting (at least on >>>>> Tegra20) and adding a proper shifting / sizes validation is much more work. >>>> >>>> Perhaps change to "As of now there is no use for the address shifting on >>>> Tegra20" if you post another revision. >>>> >>> I'll post a new revision of the series after getting comments to the all >>> patches, to not churn the ML. Thank you very much for the reviews! >>> >> >> However, given your previous comments to this patch, I'll probably add a bypass >> of the shit checking in case of IOMMU presence. >> The IOMMU presence checking probably wouldn't be enough. Better to check the Host1x version instead, to not break the non-IOMMU case on modern Tegras. > > I don't think that's needed - the firewall will deny pretty much all VIC > submissions due to is_addr_reg not being implemented so it cannot reasonably be > used on modern Tegras anyway. Either firewall should be completely avoided on newer Tegras or it should perform at least some checks and not break the newer Tegras.
diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c index 190353944d23..1a1568e64ba8 100644 --- a/drivers/gpu/host1x/job.c +++ b/drivers/gpu/host1x/job.c @@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf, if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset) return false; + /* relocation shift value validation isn't implemented yet */ + if (reloc->shift) + return false; + return true; }
Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/gpu/host1x/job.c | 4 ++++ 1 file changed, 4 insertions(+)