Re: [syzbot] [xfs?] UBSAN: array-index-out-of-bounds in xfs_attr3_leaf_add_work

[Eric Biggers]

On Thu, Jun 22, 2023 at 07:51:23PM +1000, 'Dave Chinner' via syzkaller-bugs wrote:
> > 
> > My understanding is that the main motivation for the conversions to flex arrays
> > is kernel hardening, as it allows bounds checking to be enabled.
> 
> <sigh>
> 
> Do you think we don't know this?

You said the only reason to fix this would be to "shut up UBSAN and/or syzbot".
So it seemed you were not aware of the kernel hardening motivation.

> 
> We can't actually rely on the compiler checking here. We *must* to
> do runtime verification of these on-disk format structures because
> we are parsing dynamic structures, not fixed size arrays.  IOWs, we
> already bounds check these arrays (in multiple ways!) before we do
> the memcpy(), and have done so for many, many years.
> 
> *This code is safe* no matter what the compiler says.
> 
> Last time this came up in a FORTIFY_SOURCE context, Darrick proposed
> to change this to use unsafe_memcpy() to switch off the compile time
> checking because we *must* do runtime checking of the array lengths
> provided in the structure itself.
> 
> Kees pretty much knocked that down by instead proposing some
> "flex_cpy()" API he was working on that never went anywhere. So
> kernel security people essentially said "no, we don't want you to
> fix it that way, but then failed to provide the new infrastructure
> they said we should use for this purpose.
> 
> Damned if we do, damned if we don't.
> 
> So until someone from the security side of the fence ponies up the
> resources to fix this in a way that is acceptible to the security
> people and they do all the testing and validation we require for
> such an on-disk format change, the status quo is unlikely to change.

I think you've missed the point.  The point is not about improving the bounds
checks for this specific field.  Rather, it's about eliminating a "false
positive" so that automatic bounds checking of known-sized arrays can be enabled
universally as a hardening measure.  Without code like this fixed, it will be
impossible to enable automatic bounds checking, at least in the affected files.

> 
> > You can probably get away with not fixing this for a little while longer, as
> > that stuff is still a work in progress.  But I would suggest you be careful
> > about potentially getting yourself into a position where XFS is blocking
> > enabling security mitigations for the whole kernel...
> 
> <sigh>
> 
> I'm really getting tired of all these "you'll do what we say or
> else" threats that have been made over the past few months.
> 
> As I said: if this is a priority, then the entities who have given
> it priority need to provide resources to fix it, not demand that
> others do the work they want done right now for free. If anyone
> wants work done for free, then they'll just have to wait in line
> until we've got time to address it.

Well, good news: Gustavo Silva, Kees Cook, and others are going through the
kernel and doing the conversions to flex arrays.

I am trying to help you understand the problem, not force you to fix it.  If you
do not want to fix it yourself, then you can simply consider this bug report as
a heads up.

I would just ask that you please try to be cooperative when you eventually do
get a patch from someone trying to fix it.

XFS does not need to be the "problem subsystem" every time.

- Eric