| Message ID | 54acaa00c3d44dc3b175dc55b44f1373@4rf.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Fix data abort caused by mis-aligning fit data in | expand |
On Fri, Jul 24, 2020 at 05:19:39AM +0000, Reuben Dowle wrote: > Attempting to place device tree immediately after an image in memory can lead > to mis-aligned data accesses if that image size is not divisible by the > alignment requirements of the architecture. > > Data aborts caused by this were observed on a custom Marvel A388 based system, > where the image was a uboot FIT file. The total size varies depending on the > uboot device tree size, which does not always lead to correct alignment. > > Signed-off-by: Reuben Dowle <reuben.dowle@4rf.com> > --- > common/spl/spl_fit.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c > index 365104f..d221075 100644 > --- a/common/spl/spl_fit.c > +++ b/common/spl/spl_fit.c > @@ -349,9 +349,9 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, > > /* > * Use the address following the image as target address for the > - * device tree. > + * device tree. Ensure that address is appropriately aligned. > */ > - image_info.load_addr = spl_image->load_addr + spl_image->size; > + image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, ARCH_DMA_MINALIGN); > > /* Figure out which device tree the board wants to use */ > node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, index++); Sorry for the late reply here. While we do need to ensure that the device tree is aligned, the documented alignment requirement for all platforms is 8 byte, so we should document and use that here in case future platforms require a higher alignment. Thanks!
Should I submit a new patch with the alignment set to 8 bytes? I would think a hard coded 8 bytes would not be the best solution, since not all architectures will need that much alignment. I suspect some would work with any alignment, and most 32-bit archs would be fine with 4-byte alignment. Our released software is actually using a patch to align to 4096 bytes, but I knew that was unnecessarily large. I was not really sure what would be an appropriate value here, and took a guess at ARCH_DMA_MINALIGN when I cleaned it up for submitting upstream. Is there a better define to use? I am also interested to know where the 8 byte alignment requirement is documented. Reuben > -----Original Message----- > From: Tom Rini <trini@konsulko.com> > Sent: Tuesday, 25 August 2020 2:27 am > To: Reuben Dowle <reuben.dowle@4rf.com> > Cc: u-boot@lists.denx.de > Subject: Re: [PATCH] Fix data abort caused by mis-aligning fit data in > > On Fri, Jul 24, 2020 at 05:19:39AM +0000, Reuben Dowle wrote: > > > Attempting to place device tree immediately after an image in memory > > can lead to mis-aligned data accesses if that image size is not > > divisible by the alignment requirements of the architecture. > > > > Data aborts caused by this were observed on a custom Marvel A388 based > > system, where the image was a uboot FIT file. The total size varies > > depending on the uboot device tree size, which does not always lead to > correct alignment. > > > > Signed-off-by: Reuben Dowle <reuben.dowle@4rf.com> > > --- > > common/spl/spl_fit.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index > > 365104f..d221075 100644 > > --- a/common/spl/spl_fit.c > > +++ b/common/spl/spl_fit.c > > @@ -349,9 +349,9 @@ static int spl_fit_append_fdt(struct > > spl_image_info *spl_image, > > > > /* > > * Use the address following the image as target address for the > > - * device tree. > > + * device tree. Ensure that address is appropriately aligned. > > */ > > - image_info.load_addr = spl_image->load_addr + spl_image->size; > > + image_info.load_addr = ALIGN(spl_image->load_addr + spl_image- > >size, > > +ARCH_DMA_MINALIGN); > > > > /* Figure out which device tree the board wants to use */ > > node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, > index++); > > Sorry for the late reply here. While we do need to ensure that the device > tree is aligned, the documented alignment requirement for all platforms is 8 > byte, so we should document and use that here in case future platforms > require a higher alignment. Thanks! > > -- > Tom
On Mon, Aug 24, 2020 at 08:05:24PM +0000, Reuben Dowle wrote: > Should I submit a new patch with the alignment set to 8 bytes? I would think a hard coded 8 bytes would not be the best solution, since not all architectures will need that much alignment. I suspect some would work with any alignment, and most 32-bit archs would be fine with 4-byte alignment. > > Our released software is actually using a patch to align to 4096 bytes, but I knew that was unnecessarily large. I was not really sure what would be an appropriate value here, and took a guess at ARCH_DMA_MINALIGN when I cleaned it up for submitting upstream. Is there a better define to use? > > I am also interested to know where the 8 byte alignment requirement is documented. So we're talking about the device tree file, and only that, in this part of the code, right? In the Linux kernel documentation, both arm and arm64 document that the device tree must be on an 8-byte aligned address. That is the bare minimum. If we aren't further relocating it (as fdt_high is set to 0xffffffff for example, which in general is wrong and bad), that's still the best we can do. It would be good to allow for further relocation down the line as we aren't making sure it wouldn't be overwritten by the kernel BSS, etc.
I can see that arm64 requires 8 bytes. That is stated in section 2 of https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst. I can't see a similar requirement for arm, although my search was not exhaustive. More generally I can see that all device trees must be at least 4 byte aligned (from section II.2 of https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/booting-without-of.rst) So it does seem that 8 bytes would work for at least both of these. I would be happy with hard-coding that, as I doubt it would cause any problems with other architectures. I don't have anything to add on the ability to relocate the device tree. In my case the device tree is for the next stage u-boot, so won't need relocating. This might become an issue if this was booting direct to linux from the SPL perhaps. > -----Original Message----- > From: Tom Rini <trini@konsulko.com> > Sent: Tuesday, 25 August 2020 8:40 am > To: Reuben Dowle <reuben.dowle@4rf.com> > Cc: u-boot@lists.denx.de > Subject: Re: [PATCH] Fix data abort caused by mis-aligning fit data in > > On Mon, Aug 24, 2020 at 08:05:24PM +0000, Reuben Dowle wrote: > > > Should I submit a new patch with the alignment set to 8 bytes? I would > think a hard coded 8 bytes would not be the best solution, since not all > architectures will need that much alignment. I suspect some would work with > any alignment, and most 32-bit archs would be fine with 4-byte alignment. > > > > Our released software is actually using a patch to align to 4096 bytes, but I > knew that was unnecessarily large. I was not really sure what would be an > appropriate value here, and took a guess at ARCH_DMA_MINALIGN when I > cleaned it up for submitting upstream. Is there a better define to use? > > > > I am also interested to know where the 8 byte alignment requirement is > documented. > > So we're talking about the device tree file, and only that, in this part > of the code, right? In the Linux kernel documentation, both arm and > arm64 document that the device tree must be on an 8-byte aligned > address. That is the bare minimum. If we aren't further relocating it > (as fdt_high is set to 0xffffffff for example, which in general is wrong > and bad), that's still the best we can do. It would be good to allow > for further relocation down the line as we aren't making sure it > wouldn't be overwritten by the kernel BSS, etc. > > -- > Tom
On Mon, Aug 24, 2020 at 09:18:40PM +0000, Reuben Dowle wrote: > I can see that arm64 requires 8 bytes. That is stated in section 2 of https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst. > > I can't see a similar requirement for arm, although my search was not exhaustive. More generally I can see that all device trees must be at least 4 byte aligned (from section II.2 of https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/booting-without-of.rst) > > So it does seem that 8 bytes would work for at least both of these. I would be happy with hard-coding that, as I doubt it would cause any problems with other architectures. > > I don’t have anything to add on the ability to relocate the device tree. In my case the device tree is for the next stage u-boot, so won't need relocating. This might become an issue if this was booting direct to linux from the SPL perhaps. For arm it's spelled out differently[1] as "64bit aligned address". So using 8 with a big annotated comment is probably best here. And then ah, yes, that's right, that's why we may end up in the case we're in. Thanks! [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/booting.rst#n126 > > > > ________________________________ > > [cid:4RFLogo(Custom)(2)_0f31a7de-6dd6-43cf-bc6a-a097a2b80b69.jpg] > Reuben Dowle > Software Architect > > Phone: > > Fax: > E-Mail: > Website: > > > +64 4 499 6000 > > +64 4 473 4447 > reuben.dowle@4rf.com<mailto:reuben.dowle@4rf.com> > Https://www.4rf.com<https://www.4rf.com> > > > ________________________________ > > [cid:Family_53c410b1-7227-4a5f-9acb-f09bd7617a39.png] <http://www.4rf.com/news/events> > > -----Original Message----- > > From: Tom Rini <trini@konsulko.com> > > Sent: Tuesday, 25 August 2020 8:40 am > > To: Reuben Dowle <reuben.dowle@4rf.com> > > Cc: u-boot@lists.denx.de > > Subject: Re: [PATCH] Fix data abort caused by mis-aligning fit data in > > > > On Mon, Aug 24, 2020 at 08:05:24PM +0000, Reuben Dowle wrote: > > > > > Should I submit a new patch with the alignment set to 8 bytes? I would > > think a hard coded 8 bytes would not be the best solution, since not all > > architectures will need that much alignment. I suspect some would work with > > any alignment, and most 32-bit archs would be fine with 4-byte alignment. > > > > > > Our released software is actually using a patch to align to 4096 bytes, but I > > knew that was unnecessarily large. I was not really sure what would be an > > appropriate value here, and took a guess at ARCH_DMA_MINALIGN when I > > cleaned it up for submitting upstream. Is there a better define to use? > > > > > > I am also interested to know where the 8 byte alignment requirement is > > documented. > > > > So we're talking about the device tree file, and only that, in this part > > of the code, right? In the Linux kernel documentation, both arm and > > arm64 document that the device tree must be on an 8-byte aligned > > address. That is the bare minimum. If we aren't further relocating it > > (as fdt_high is set to 0xffffffff for example, which in general is wrong > > and bad), that's still the best we can do. It would be good to allow > > for further relocation down the line as we aren't making sure it > > wouldn't be overwritten by the kernel BSS, etc. > > > > -- > > Tom > ________________________________ > The information in this email communication (inclusive of attachments) is confidential to 4RF Limited and the intended recipient(s). If you are not the intended recipient(s), please note that any use, disclosure, distribution or copying of this information or any part thereof is strictly prohibited and that the author accepts no liability for the consequences of any action taken on the basis of the information provided. If you have received this email in error, please notify the sender immediately by return email and then delete all instances of this email from your system. 4RF Limited will not accept responsibility for any consequences associated with the use of this email (including, but not limited to, damages sustained as a result of any viruses and/or any action or lack of action taken in reliance on it).
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 365104f..d221075 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -349,9 +349,9 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, /* * Use the address following the image as target address for the - * device tree. + * device tree. Ensure that address is appropriately aligned. */ - image_info.load_addr = spl_image->load_addr + spl_image->size; + image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, ARCH_DMA_MINALIGN); /* Figure out which device tree the board wants to use */ node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, index++);
Attempting to place device tree immediately after an image in memory can lead to mis-aligned data accesses if that image size is not divisible by the alignment requirements of the architecture. Data aborts caused by this were observed on a custom Marvel A388 based system, where the image was a uboot FIT file. The total size varies depending on the uboot device tree size, which does not always lead to correct alignment. Signed-off-by: Reuben Dowle <reuben.dowle@4rf.com> --- common/spl/spl_fit.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)