On 4/15/22 09:30, Niklas Cassel wrote:
> On Fri, Apr 15, 2022 at 08:51:27AM +0900, Damien Le Moal wrote:
>> On 4/14/22 18:10, Niklas Cassel wrote:
>
> (snip)
>
>> This looks good to me. But thinking more about it, do we really need to
>> check what the content of the header is ? Why not simply replace this
>> entire hunk with:
>>
>> return rp + sizeof(unsigned long) * 2;
>>
>> to ignore the 16B (or 8B for 32-bits arch) header regardless of what the
>> header word values are ? Are there any case where the header is *not*
>> present ?
>
> Considering that I haven't been able to find any real specification that
> describes the bFLT format. (No, the elf2flt source is no specification.)
> This whole format seems kind of fragile.
>
> I realize that checking the first one or two entries after data start is
> not the most robust thing, but I still prefer it over skipping blindly.
>
> Especially considering that only m68k seems to support shared libraries
> with bFLT. So even while this header is reserved for ld.so, it will most
> likely only be used on m68k bFLT binaries.. so perhaps elf2flt some day
> decides to strip away this header on all bFLT binaries except for m68k?
>
> bFLT seems to currently be at version 4, perhaps such a change would
> require a version bump.. Or not? (Now, if there only was a spec.. :P)
The header skip is only for riscv since you have that
"if (IS_ENABLED(CONFIG_RISCV)) {". So whatever you do under that if will
not affect other architectures. The patch will be a nop for them.
So if we are sure that we can just skip the first 16B/8B for riscv, I
would not bother checking the header content. But as mentioned, the
current code is fine too.
Both approaches are fine with me but I prefer the simpler one :)
>
>
> Kind regards,
> Niklas
>
>>
>>> + }
>>> + return rp;
>>> +}
>>> +
>>> static int load_flat_file(struct linux_binprm *bprm,
>>> struct lib_info *libinfo, int id, unsigned long *extra_stack)
>>> {
>>> @@ -789,7 +813,8 @@ static int load_flat_file(struct linux_binprm *bprm,
>>> * image.
>>> */
>>> if (flags & FLAT_FLAG_GOTPIC) {
>>> - for (rp = (u32 __user *)datapos; ; rp++) {
>>> + rp = skip_got_header((u32 * __user) datapos);
>>> + for (; ; rp++) {
>>> u32 addr, rp_val;
>>> if (get_user(rp_val, rp))
>>> return -EFAULT;
>>
>> Regardless of the above nit, feel free to add:
>>
>> Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
--
Damien Le Moal
Western Digital Research
